Repository navigation
[fix](fe) Keep an ADBC catalog's driver open until the calls still using it finish - #69874
Merged
Merged
Conversation
…ing it finish ### What problem does this PR solve? Issue Number: None Related PR: apache#66331, apache#68353 Problem Summary: The External regression pipeline loses its FE in the middle of a run, for PRs that do not touch ADBC: 11 of the 834 runs between 2026-10-22 and 2026-10-30. The FE dies with a SIGSEGV or a glibc abort inside libadbc_driver_sqlite.so, and every suite after that fails with "Communications link failure" (54 to 56 cases per run). The native frame differs from run to run: nanoarrow::ArrayStreamFactory<VectorArrayStream>::get_next_wrapper+0x4 (6 runs), PrivateArrowSchemaDeepCopy+0x46 (2), "double free or corruption (fasttop)" (2), no message (1); an earlier run died in sqlite3FindTable. All of them die at the same moment: test_adbc_sqlite_catalog_scan drops a catalog that has the metadata cache turned off ("meta.cache.adbc.metadata.enable" = "false") right after querying it. In the same millisecond fe.log shows the catalog's Arrow allocator closing with "Memory was leaked by query", and a column-statistics load failing with "Failed to read the ADBC getObjects result: ArrowArrayStream is already closed". Root cause: the query started column-statistics loads on the STATS_FETCH pool. A load that finds nothing in the statistics table asks the table itself, which for an ADBC table goes through PluginDrivenExternalTable.getColumnStatistic -> AdbcConnectorMetadata.getTableHandle -> AdbcClient.withConnection -> getObjects. With the metadata cache off, it reaches the driver every time. DROP CATALOG (and ALTER CATALOG ... SET PROPERTIES, which resets the catalog) closes the catalog's connector on another thread, and AdbcClient.close() closed the native AdbcDatabase and the allocator at once. In adbc-driver-jni, JniDatabase.close() first closes every connection still open on the database, and each connection first closes the results still open on it, all on the closing thread. The JNI bridge only checks a handle before a call starts, and the C driver does not check at all. So a statistics load that is reading a getObjects result, or is inside the driver on its connection, uses native memory that was just freed. Where it dies depends on where that thread is at that moment. A thread that comes back only after the close has finished gets a Java exception instead: that is the "ArrowArrayStream is already closed" in the log. Fix: AdbcClient counts the calls inside withConnection. close() now only refuses new calls. It releases the database and the allocator itself when no call is in flight. Otherwise the last call to leave releases them. A failure of that deferred release is logged with the driver's path, as the catalog logs a failed connector close, rather than thrown at a caller that did not close anything: that call's own work succeeded. close() does not wait for the calls in flight, so DROP and ALTER CATALOG are not held up by a slow remote source. ### Release note Fix an FE crash (SIGSEGV in the ADBC driver) when an ADBC catalog is dropped or altered while a statement or a statistics load is still reading it. ### Check List (For Author) - Test: Unit Test, Manual test - AdbcClientTest +5, run with the real JNI bridge and SQLite driver (0 skipped; all 197 tests of fe-connector-adbc pass): a call still on its connection when the client is closed keeps using it; a call still reading a getObjects result when the client is closed reads it to the end; the last call to leave a closed client releases the database exactly once and only after it leaves; a release that fails as the last call leaves does not fail that call; an idle client releases at once. With close() put back to releasing at once, the first four fail ("Native connection handle is closed", "Attempting operation on allocator when allocator is closed", "the database was released under an open connection"). - Manual test: a stress program (not committed) closes the client 200 times while four threads loop over getObjects and getTableSchema through withConnection. With the AdbcClient of master, 5 of 5 runs crashed the JVM about a second after it started (SIGSEGV in PrivateArrowSchemaDeepCopy, under AdbcObjectsReader.readNamespaces). With this change, 5 of 5 runs completed all 200 closes (6,019 to 7,238 calls each), no call failed because the close reached it, and no close or deferred release failed (checked with WARN logging on, where the unit test's deliberate release failure shows up as one warning). - Behavior changed: No (DROP/ALTER CATALOG still return without waiting; the native driver of an ADBC catalog is now freed when its last in-flight call finishes instead of under it) - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed PR 69874 at the verified head 50196fe. The production lifecycle change meets its goal: every successful AdbcClient enter is paired with one leave across connection, body, and close failures; close marks the client closed before deciding whether to defer; release detaches ownership under the monitor before native close; and all metadata, scan, and catalog teardown paths keep borrowed ADBC children inside withConnection. I found no additional production concurrency, lifecycle, error-handling, compatibility, or downstream-path defect.
The accepted issue is test placement. The new closeLeavesAResultACallIsStillReadingAlone test iterates a real native ArrowReader in the FE connector unit-test module. AdbcNativeTestSupport explicitly documents that ArrowReader tests belong in regression-test/suites/external_table_p0/adbc because the arrow-c-data JNI shim requires CXXABI_1.3.9 and causes FE unit-test failures on supported CentOS 7 hosts. Move that test to the regression suite or replace the native reader with a non-native fake.
Critical checkpoints: the task goal is implemented in production; the code remains focused; synchronized state and detached cleanup avoid release races; no configuration, protocol, persistence, or storage-format changes are involved; and the added tests were inspected for API compatibility and lifecycle coverage. Tests were not executed under this static-only review. No user-provided focus points were supplied. No existing P0/P1 inline findings apply. The review converged after round 1.
Review converged after 1 round(s).
| @@ -74,6 +89,139 @@ void usingAClosedClientFailsLoud(@TempDir Path tempDir) { | |||
| Assertions.assertTrue(e.getMessage().contains("closed"), e.getMessage()); | |||
| } | |||
Contributor
There was a problem hiding this comment.
[P1] Move this real ArrowReader materialization test out of FE unit tests (or replace it with a non-native fake). AdbcNativeTestSupport explicitly documents that tests which iterate an ArrowReader must live in the ADBC regression suites because the arrow-c-data JNI shim requires CXXABI_1.3.9 and makes FE unit tests fail on supported CentOS 7 hosts. This test calls getObjects and AdbcObjectsReader.readNamespaces on the native SQLite path, so the connector unit suite will fail on those hosts before it can verify the lifecycle fix.
Contributor
Author
There was a problem hiding this comment.
Fixed in 2db8c88 with the second option. The case now runs on the recording client: the recording connection answers getObjects with SQLite's answer, built in memory by AdbcObjectsReaderTest.response (now package-private) from the allocator the client hands to openDatabase, which is the allocator a driver's result lives in. Nothing on that path touches org.apache.arrow.c. With the shim deleted from a copy of the arrow-c-data jar, standing in for the FE UT agent, the case as you reviewed it fails at its first read, as it did in FE UT build 1086409, while with this commit all 197 tests of the module pass.
The case keeps the coverage the native version had. With close() releasing at once, it fails along with the other three new cases ("the database was released under an open connection"). With only the allocator freed at once and the database left to the last call, it is still the only case in the class that fails ("Attempting operation on allocator when allocator is closed"). That is why I rewrote it rather than dropped it: the recording cases never allocate, and the native getCurrentCatalog case only reads a Java string.
I did not move it to the regression suites. A suite has no way to hold a statistics load between getObjects returning and the read, because the connector has no pause point to hold it at: DebugPointUtil lives in fe-common, which fe-connector-adbc does not depend on. The end-to-end form of this race is already covered, though not deterministically: test_adbc_sqlite_catalog_scan drops its _nocache catalog right after querying it, which is where the 11 External FE crashes in the description came from.
Contributor
TPC-H: Total hot run time: 28593 ms |
Contributor
TPC-DS: Total hot run time: 150890 ms |
Contributor
ClickBench: Total hot run time: 24.19 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
… memory ### What problem does this PR solve? Issue Number: None Related PR: apache#68510 Problem Summary: AdbcClientTest.closeLeavesAResultACallIsStillReadingAlone checks that a call still reading a getObjects result when its AdbcClient is closed reads that result to the end. It read the result from the real SQLite driver. Reading a driver's result into Arrow loads arrow-c-data's own JNI shim, an upstream binary that needs CXXABI_1.3.9, and the FE UT agent's /lib64/libstdc++.so.6 does not have it. So the case failed FE UT at its first read with "UnsatisfiedLinkError: ... version `CXXABI_1.3.9' not found", before it could check anything about closing. It was the only failure of FE UT build 1086409 that counted; the other 11 cases of the class passed there, the native ones included. AdbcNativeTestSupport has stated the rule since apache#68510: tests that iterate an ArrowReader do not belong in this module's test sources. The case now runs on the recording client. The recording connection answers getObjects with SQLite's answer, built in memory by AdbcObjectsReaderTest.response (package-private now, with catalog and schema) from the allocator the client hands to openDatabase, which is the allocator a driver's result lives in. The case still checks what it was added for. It is also still the only case in the class that fails when close() frees the allocator at once and leaves only the database to the last call, which is why it is rewritten rather than dropped: the recording cases never allocate, and the native getCurrentCatalog case reads a Java string. ### Release note None ### Check List (For Author) - Test: Unit Test - fe-connector-adbc: all 197 tests pass, 0 skipped, with the real JNI bridge and SQLite driver; checkstyle reports no violations. - With the shim deleted from a copy of the arrow-c-data jar, standing in for the FE UT agent: the case as it was fails at its first read while the other 11 cases of the class pass, as on the agent; with this commit all 197 tests pass. - Mutations of AdbcClient.close(): releasing at once fails the same 4 cases as before this commit, this one included ("the database was released under an open connection"); freeing the allocator at once and deferring only the database fails this case alone ("Attempting operation on allocator when allocator is closed"), as it did before this commit. That message is checked only with Java assertions on, which surefire enables by default and no pom here turns off. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all three changed files at 2db8c88 and the metadata/statistics, scan-planning and DROP/ALTER cleanup callers. No new actionable findings. The review converged in one round: both complete-review agents and the independent lifecycle-risk agent returned NO_NEW_VALUABLE_FINDINGS, and all risk hypotheses were independently resolved.
Existing P1 4420412731 is addressed. The result-reading test now uses RecordingClient and an Arrow IPC reader built with the client's allocator, avoiding the native C-data reader. Reading starts after close returns and still allocates through that allocator, preserving early-release coverage (Arrow 19 IPC allocation). No existing P0/P1 finding remains applicable; no duplicate inline comment is posted. No additional user-provided focus was specified.
Critical checkpoint conclusions:
- Goal: the counted lease protects an admitted call through connection/result cleanup. Close refuses subsequent calls and leaves release to the last active caller. The five added tests express the intended ordering; they were inspected, not executed.
- Scope and clarity: one production class and two related test classes; the package-private database factory and fixture visibility changes support recording tests without expanding the connector SPI.
- Concurrency: statistics/planning threads can race catalog reclamation. The client monitor guards closed, count, database and allocator. Connect, callback, connection close and resource destruction occur outside it. Cold initialization already performed I/O under this monitor. No new lock-order inversion was found.
- Lifecycle and memory: reader/statement/connection cleanup precedes decrement. Closed admission prevents reopening, and atomic detachment gives the database/allocator pair to one releaser even with competing close/last-leave calls. Failed entry, nested calls and exceptional exits were checked. No new ownership cycle or static-initialization dependency was found. Deferred native cleanup uses an already-captured closer (ADBC native handle); the inspected close chain does not depend on the restored TCCL.
- Configuration: no settings or dynamic-update behavior changed.
- Compatibility: no shared SPI, stored format or rolling-upgrade protocol change.
- Thrift/protobuf: no definitions or field IDs changed.
- Parallel paths: metadata listings, schema fallback, vendor probe, validation and partition planning all use the lease. Returned schemas/names are Java data; partition descriptors own Java byte arrays (ADBC partition result). No native reader escapes its callback.
- Conditions and errors: closed/null/count checks represent reachable lifecycle states. Driver errors retain translation, allocator cleanup remains in finally, idle-close failures propagate, and deferred-release failures are warned without replacing completed call results.
- Test coverage: latch/Future synchronization covers active connection and reader preservation, late rejection, last-call release, repeated close, release-failure isolation and idle release. The existing cache-disabled SQLite query/drop regression path was also inspected. Multi-caller and connect/callback/connection-close failure interleavings were code-reviewed but are not directly added as test cases.
- Test results: expected-result files are unchanged; new assertions agree with the IPC fixture's namespace and close counts.
- Observability: deferred failures include driver path and cause at WARN. No new high-volume logging or additional metric requirement was identified.
- Transactions and persistence: no EditLog, replay, commit/visibility or failover semantics changed; opening remains lazy.
- Data writes and atomicity: no data-write path changed; rowset/version/delete-bitmap invariants are unaffected. The relevant allocator/native ownership was checked above.
- FE/BE propagation: no cross-process variables or send paths changed.
- Performance: constant-time monitor admission/exit per metadata call; no new row loop, copy or remote call in production, and destruction runs outside the client monitor.
- Other issues: no additional verified defect after the final changed-file, candidate and risk sweep.
Limits: static review only. No builds, unit/regression tests, stress runs or native/platform execution were performed, and the author's reported execution results were not independently reproduced. No unresolved review candidate or uncovered changed-code scope remains.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28282 ms |
Contributor
TPC-DS: Total hot run time: 151736 ms |
Contributor
ClickBench: Total hot run time: 24.39 s |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
CalvinKirs
approved these changes
Oct 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #66331, #68353
Problem Summary:
In short: dropping or altering an ADBC catalog can crash the FE process. Closing the catalog frees the native ADBC objects at once, including the connections and results that other threads are still reading, such as the column-statistics loads that a query on the catalog has just started. This kills the FE in about one External regression run in 75, whatever the PR under test changes. With this PR, closing a client only refuses new calls, and the last call still in flight releases the driver as it leaves. DROP and ALTER CATALOG still return without waiting.
This commit was first pushed as the last commit of #68353, which is still in review. It shares nothing with the rest of that PR, so it is carved out here to land on its own.
Background
An ADBC catalog (#66331) reads its metadata through
AdbcClient, which owns one nativeAdbcDatabaseand one Arrow allocator per catalog. Every metadata call borrows a fresh connection throughAdbcClient.withConnectionand closes it afterwards. FE talks to the C driver through adbc-driver-jni 0.24. In that bridge, closing a parent closes everything still open under it, on the closing thread:JniDatabase.close()closes every connection still open on the database, andJniConnection.close()closes every result (getObjectsand similar readers) still open on the connection. The bridge only checks a handle before a call starts, and the C driver does not check at all, so freeing an object that another thread is still using is a use-after-free in native code, which takes the whole JVM down rather than failing one call.Column statistics reach the driver too. When a query plans against an ADBC table, the STATS_FETCH pool loads column statistics in the background. A load that finds nothing in the statistics table asks the table itself:
PluginDrivenExternalTable.getColumnStatistic→AdbcConnectorMetadata.getTableHandle→withConnection→getObjects. With"meta.cache.adbc.metadata.enable" = "false", every such load calls into the driver.The problem, and what it cost
AdbcClient.close()closed the database and then the allocator immediately, whatever was still in flight. DROP CATALOG and ALTER CATALOG ... SET PROPERTIES close the catalog's connector this way, on their own thread (since [improvement](fe) Defer external catalog resource cleanup from replay #69665, on theExternalResourceReclaimerpool; before that, synchronously on the statement thread, with the same race).getObjectsresult, or was inside the driver on its connection, then read memory that had just been freed. Where it dies depends on where that thread is at that moment.Communications link failure, so the PR under test showed a red run it did not cause. All 11 died at the same moment:test_adbc_sqlite_catalog_scandrops its catalog with the metadata cache turned off right after querying it. In the same millisecond, fe.log shows the allocator closing withMemory was leaked by queryand a statistics load failing withFailed to read the ADBC getObjects result: ArrowArrayStream is already closed. That is the same race, caught by a thread that came back only after the close had finished.fe.outSIGSEGVinnanoarrow::ArrayStreamFactory<VectorArrayStream>::get_next_wrapper+0x4SIGSEGVinPrivateArrowSchemaDeepCopy+0x46double free or corruption (fasttop)SIGSEGVinsqlite3FindTable(an earlier run, 2026-10-06)How this PR fixes it
AdbcClientcounts the calls insidewithConnection(enter/leave, under the client's monitor, which also opens the database on first use).close()marks the client closed, so new calls are refused withAdbcClient has been closed. It releases the database and the allocator itself only when no call is in flight. Otherwise it returns at once, and the last call to leave releases them.close()never waits for the calls in flight, so DROP and ALTER CATALOG are not held up by a slow remote source.Results
Unit tests (0 skipped). The first case runs against the real JNI bridge and the SQLite driver, the others against a recording database:
AdbcClientTestcaseclose()releasing at once, as before this PRNative connection handle is closedgetObjectsresult when the client is closed reads it to the endthe database was released under an open connectionthe database was released under an open connectionthe database was released under an open connectionFreeing only the allocator at once, and leaving the database to the last call, fails the
getObjectscase alone:Attempting operation on allocator when allocator is closed.A stress program (not committed) closes a client 200 times while four threads loop over
getObjectsandgetTableSchemathroughwithConnectionagainst the SQLite driver:AdbcClientof masterSIGSEGVinPrivateArrowSchemaDeepCopy+0x40, underArrowArrayStreamReader.readSchema←AdbcObjectsReader.readNamespaces←AdbcClient.withConnectionThe stress runs used the macOS arm64 build of the same SQLite driver, so the offset differs from the Linux one in CI.
Classes, and how they call each other
AdbcClient:withConnectionbrackets the body withenter/leave;closerefuses new calls and releases only when idle;releasecloses the database, then the allocator;openDatabaseis the one place the database is created, so a test can stand a recording database in for the driver.AdbcClientTest: five new cases, one against the real driver and four against a recording database. The recording connection answersgetObjectswith a result built in memory in the client's allocator: reading a driver's result into Arrow loads arrow-c-data's JNI shim, which needs CXXABI_1.3.9 and does not load on the FE UT agent (seeAdbcNativeTestSupport, [test](fe) Run the ADBC connector unit tests in FE UT without the arrow-c-data JNI shim #68510).AdbcObjectsReaderTest:response,catalogandschemaare package-private, soAdbcClientTestbuilds that result with them.Not changed:
AdbcConnector, the catalog lifecycle andExternalResourceReclaimer, BE, and the regression suites;test_adbc_sqlite_catalog_scanstays as it is, since what it exposes is this bug.Release note
Fix an FE crash (SIGSEGV in the ADBC driver) when an ADBC catalog is dropped or altered while a statement or a statistics load is still reading it.
Check List (For Author)
Test
Unit test:
AdbcClientTest, 12 cases including the five new ones, and all 197 tests offe-connector-adbcpass with the real JNI bridge and SQLite driver, and still pass with arrow-c-data's JNI shim deleted from its jar, standing in for the FE UT agent, where the shim does not load. The four new cases above fail withclose()put back to releasing at once; freeing only the allocator at once fails thegetObjectscase. Manual: the stress program in the table above, run against theAdbcClientof master and against this PR.Behavior changed:
DROP and ALTER CATALOG still return without waiting. The native driver of an ADBC catalog is now freed when its last in-flight call finishes, instead of under it.
Does this need documentation?
Check List (For Reviewer who merge this PR)