Repository navigation
[fix](fe) Stop leaking connector scope state after a statement ends - #69718
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#69298, apache#69566, apache#66530 Problem Summary: A statement's connector state could outlive the statement in two ways. 1. A split producer that is still inside a read when the statement ends keeps going: stopping a scan only flags its producers, and closing the query's task groups does not interrupt metadata IO. After closeAll() it resolved its table again through the closed connector statement scope, which closeAll() had emptied, so Iceberg borrowed a new table lease and Hive partition batches built a new ConnectorMetadata, and both stayed in the closed scope for good: the lease was never returned. A value whose load was in flight while closeAll() ran was missed the same way. This happens whenever a statement ends while a producer reads: an EXPLAIN of a large table, a LIMIT that finishes early, a KILL or a timeout. 2. EXPLAIN REFRESH MATERIALIZED VIEW plans on an MTMV context of its own that had no query id, so its scans registered their query-finish cleanup under "" and its StatementContext was never closed: every such EXPLAIN over an external base table left a registry entry and an open connector scope behind. A closed ConnectorStatementScopeImpl now refuses to load: computeIfAbsent throws CancellationException without running the loader (as the query's task groups already do once closed), a load that raced the close is released by its caller, and closeAll() removes each entry before releasing it, so every value is released exactly once. A session built after the scope closed (the lineage of an INSERT, say) captures NONE and the direct scheduler, as offline work does, instead of failing in the closed scope. ExplainCommand gives an explain context of its own a query id and, when the EXPLAIN ends, runs that query id's callbacks and closes the context's StatementContext, as MTMVTask does for a refresh. Found while checking master against branch-4.x's apache#66473; branch-4.x has neither the connector statement scope nor the query-finish registry. ### Release note None ### Check List (For Author) - Test: Unit Test - ConnectorStatementScopeTest, SplitProducerTest and ExplainRefreshMTMVCommandTest gain 5 tests; 51 related FE test classes (457 tests) pass - Behavior changed: Yes. A connector statement scope refuses to load once the statement has closed it; a connector session built after that runs without a statement scope. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: workspace routing discovery unauthorized (401) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-H: Total hot run time: 28811 ms |
TPC-DS: Total hot run time: 151527 ms |
ClickBench: Total hot run time: 24.34 s |
|
/review |
|
Codex automated review failed and did not complete. Error: workspace routing discovery unauthorized (401) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Reviewed all 8 changed files at head 0f8374ca590bbeff28f03d38a9656f880a115c36 using the supplied aggregate diff. No actionable new findings and no existing blocking inline comments. The review converged in 1 round: both complete-review agents and the independent risk reviewer returned NO_NEW_VALUABLE_FINDINGS; the main reviewer independently verified the investigated mechanisms. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and implementation: closed scopes refuse retained-session loads; conditional removal assigns release ownership for a value published during closure. A separate EXPLAIN REFRESH context receives its own query id before planning and is finished on success or failure. The added unit tests cover these intended behaviors by inspection.
- Scope, clarity and reuse: the changes are focused on statement lifetime and reuse existing query-finish callbacks, StatementContext cleanup and a common release helper.
- Concurrency and locking: traced executor cleanup against retained streaming/partition-batch producers and task groups. The volatile flag and conditional map removals prevent duplicate ownership in the reviewed interleavings. Metadata IO remains inside the existing map loader; same-bin cleanup waiting also existed through the previous cache.clear(). No introduced lock-order defect was established; cleanup is not claimed to be wait-free.
- Lifecycle: retained producers keep their original scope; later independent sessions select NONE/DIRECT. Operation-local cross-statement scopes remain explicitly owned. Retries reset scope, prepared executions replace their context, and deferred Arrow Flight retains its existing cleanup boundary. EXPLAIN's fresh context is closed before restoring the previous thread-local, including exceptional exits. No cross-translation-unit initialization is involved.
- Configuration: no new or modified configuration items.
- Compatibility: the SPI edit is documentation only; public signatures, recorded API surface and stored formats do not change. The newly documented cancellation behavior was traced through its consumers.
- Thrift/protobuf: no definition changes or field-id review required.
- Parallel paths: checked synchronous scans, streaming and partition-batch producers, ordinary EXPLAIN, COMPLETE/INCREMENTAL refresh explanations, attempt retries, prepared execution and explicit background scopes. No missing equivalent fix was identified.
- Conditions: closed-scope checks address concrete late producer loads; context identity distinguishes ownership. Existing target-table and thread-local restoration guards retain their purpose.
- Test coverage: inspected the new JUnit 5 closed-scope, in-flight publication, offline-session, late-producer and EXPLAIN success/failure cases, plus adjacent reset/transaction-order coverage. Concurrency tests use latches and bounded waits. Real connector integration, populated-bin stress and cleanup-failure execution were assessed through code paths, not executed.
- Test results: no regression result files changed. Assertions match the reviewed ownership and callback ordering; this review does not attest that tests pass.
- Observability and errors: cancellation includes the scope key; per-value release and callback failures retain existing warning isolation. No new critical distributed operation requires additional metrics here.
- Transactions and persistence: transaction holders are created/begun on the executor thread, rather than by surviving read producers. The reviewed reachable cleanup paths preserve transaction finalization before shared metadata close. No EditLog, replay or failover protocol changes.
- Writes and atomicity: no new write execution path; EXPLAIN teardown does not execute a refresh or commit writes. No new crash-consistency or atomicity issue identified.
- FE/BE passing: no new transmitted variables or send paths.
- Performance and memory: lookups add bounded closed-state checks, cleanup remains linear in scope entries, and EXPLAIN adds one query id and callback drain. No additional scan materialization or new retained state was identified.
- Further recheck: reconsidered late fresh sessions, multiple loader waiters, planning-key cleanup, transaction ordering, callback initialization timing and failure restoration. Both initial risk areas were dismissed with code evidence; no unresolved candidate or uncovered changed file remains.
Validation limit: static review only. No builds, tests, benchmarks, runtime probes, source modifications or submodule initialization were performed. Convergence describes completion of the static review; it does not establish runtime validation.
Review converged after 1 round(s).
924060929
left a comment
There was a problem hiding this comment.
External scans share metadata, table leases and task groups through a statement scope. This change prevents late producers from loading into a closed scope and gives an independently owned EXPLAIN REFRESH context its own query id and deterministic cleanup.
Reviewed all 8 changed files and relevant FE call chains against the supplied merge base. No actionable new P0/P1/P2 findings. Scope ownership, concurrent load/close handling, transaction teardown ordering, retries, prepared execution, cancellation propagation and the added tests were inspected. No wire-format or FE/BE parameter changes.
APPROVE within the FE static-review scope. The supplied current-head P0 checks succeeded, and the existing same-head AI review reports no blocking findings. FE coverage failed; required-check rules are unknown, so this is not confirmation that all merge requirements passed. No builds or tests were run.
Automated FE-focused static review (GPT-6.1 Sol, medium). No build or tests were run.
…69894) ### What problem does this PR solve? Issue Number: N/A Problem Summary: Two FE unit tests depend on asynchronous work without owning its complete lifecycle: - `SplitProducerTest.readThatOutlivesItsStatementLoadsNothingIntoTheClosedScope` waits for the source-close signal and immediately checks the producer completion count. `SplitProducer.finish` invokes the completion callback after closing the source, so the assertion can run before that callback. Wait for the assignment termination future, which completes after both cleanup and the callback. - `StmtExecutorTest.testSuccessfulRetryRendersMergedProfileAndActualRows` registers its profile with the global background `ProfileManager` while also writing and reading the profile synchronously. Use a real manager whose daemon is not started, scoped to this test, so automatic spill, memory release and archival cannot interfere with the temporary storage lifecycle. Preserve the real persistence path and all merged-profile and actual-row assertions, and include the readback content in the failing assertion. The tests originated in #69718 and #68658 respectively. These test-only changes are independent of the Coordinator refactoring in #69830. The exact CI interleaving was not reproduced locally; both original tests also passed local repeated execution. No production behavior changes. ### Release note None ### Check List (For Author) - Test: On this independent master-based branch, `./run-fe-ut.sh --run 'SplitProducerTest,StmtExecutorTest'` passed all 81 cases (26 + 55). Before extraction, both target cases also passed 50 repetitions each (179 executions across both classes); FE Checkstyle passed with 0 violations. - Behavior changed: No - Does this need documentation: No
What problem does this PR solve?
Issue Number: None
Related PR: #69298, #69566, #66530
Problem Summary:
In short
A statement's connector state could outlive the statement in two ways on master. A split producer still inside a read when the statement ends went on to load values into the already-closed connector statement scope, such as an Iceberg table borrowed from the table cache, and nothing ever closed them.
EXPLAIN REFRESH MATERIALIZED VIEWplans on a context of its own that has no query id, so its scans registered their query-finish cleanup under""and its connector scope was never closed: each such EXPLAIN over an external base table left one registry entry and one open scope behind, without bound. This PR makes a closed scope refuse to load, and finishes the explain context's own statement when the EXPLAIN ends.This was found while checking master against branch-4.x's #66473 (scan reuse, ported as #66530). branch-4.x has neither the connector statement scope nor the query-finish registry, and its statement resources already refuse registration once closed.
Background
ConnectorStatementScopeImpl, hung on theStatementContext). Connectors keep what the statement shares in it: theConnectorMetadataof each catalog, Iceberg table leases borrowed from the table cache (returning one is what lets the cache close the table's FileIO when it evicts it), Iceberg tracked tables, write transactions, and the query's task groups since [improvement](datasource) Bound and fairly schedule external scan planning #69298.closeAll()releases them when the statement ends.QueryFinishCleanup) releases a read transaction and closes the scope. [improvement](datasource) Bound and fairly schedule external scan planning #69298 runs them at the end of every attempt, also for EXPLAIN and for plans that fail; [fix](fe) Finish a failed prepared EXECUTE like a failed COM_QUERY #69566 does the same for prepared EXECUTE.streamSplits, which resolves its table through the scope.The problem, and what it cost
SplitProducer.close()), and closing the query's task groups cancels queued work but deliberately does not interrupt metadata IO. A producer already insidegetNextBatchkeeps going aftercloseAll().closeAll()had also cleared the scope, so the producer found nothing and loaded again: Iceberg borrowed a new table lease, Hive partition batches built a newConnectorMetadata. Both stayed in the closed scope forever, so the lease was never returned. A value whose load was in flight whilecloseAll()ran was missed the same way. This happens whenever a statement ends while a producer is still reading: an EXPLAIN of a large table (EXPLAIN also starts batch scans), a LIMIT that finishes early, a KILL, a timeout. Nothing fails and no result is wrong; the FE just keeps the leases, metadata and FileIO alive.EXPLAIN REFRESH MATERIALIZED VIEWleaks its own statement. It plans on an MTMV context of its own (RefreshMTMVCommand.getExplainConnectContext), and that context had no query id, so the connector sessions of its scans had""as their query id and registered their cleanup under"". The end of the outer statement finishes only the outer query id; nothing finishes"", and it could not be finished at the end of one statement anyway, since every context without a query id shares it. The explain context'sStatementContextwas never closed either.EXPLAIN REFRESH MATERIALIZED VIEWover an external base table""and one open connector scope per executionHow this PR fixes it
ConnectorStatementScopeImpl: aftercloseAll()the statement is over, socomputeIfAbsent(and with itcomputeIfAbsentForPlanningandgetOrCreateMetadata) does not run the loader and throwsCancellationException, as the query's task groups already do once closed; Iceberg's planning paths treat it as "the query stopped" and do not fall back to the SDK. A load that raced the close is released by its caller, becausecloseAll()could not see it.closeAll()now removes each entry before releasing its value, still finalizing transactions first, so every value is released exactly once.getIfPresentreturns nothing once closed.StatementContext.getConnectorStatementScopeForNewSession(): a session built on a statement whose scope is already closed (the lineage of an INSERT, after the insert finished its query, say) capturesConnectorStatementScope.NONEand the direct task scheduler, as offline work does, instead of failing in or loading into the closed scope. Sessions captured before the close keep the closed scope, so a producer holding one stops at its next load.ExplainCommand: an explain context of its own gets a query id, and when the EXPLAIN ends, whether planning succeeded or not, it runs that query id's query-finish callbacks and closes the context'sStatementContext, asMTMVTaskdoes for a refresh. The thread-local context is still restored if that cleanup throws.closeAll()runs after the producers have quiesced are corrected; the SPI javadoc ofcomputeIfAbsentstates the closed-scope behavior.Results
CancellationException, nothing borrowedcloseAll()EXPLAIN REFRESH MATERIALIZED VIEW""and open scope left behindNew tests:
ConnectorStatementScopeTest:closedScopeRefusesToLoadAndHandsOutNothing,valueLoadedWhileTheScopeClosesIsReleasedOnceByItsCaller(the loader blocks on another thread whilecloseAll()runs),sessionBuiltAfterTheScopeClosesRunsAsOfflineWork.SplitProducerTest.readThatOutlivesItsStatementLoadsNothingIntoTheClosedScope: a producer is stopped insidegetNextBatch, the scope closes, then the read resolves its table.ExplainRefreshMTMVCommandTest.testExplainCommandFinishesTheStatementOfItsOwnContext: planning registers a callback under the explain context's query id and keeps a closeable in its scope; after the EXPLAIN, both succeeding and failing, the callback ran once and the value was closed.Each new test was checked against a mutation of the code it guards; every mutation turns its test red:
closedScopeRefusesToLoadAndHandsOutNothing(the loader ran twice),readThatOutlivesItsStatementLoadsNothingIntoTheClosedScope(the late read loaded once)valueLoadedWhileTheScopeClosesIsReleasedOnceByItsCaller(the caller got the value)sessionBuiltAfterTheScopeClosesRunsAsOfflineWork(building the session throws: the scope cannot loadengine:query-task-groups)testExplainCommandFinishesTheStatementOfItsOwnContext(callback never ran)testExplainCommandFinishesTheStatementOfItsOwnContext(planned without a query id)Release note
None
Check List (For Author)
TZ=Asia/Shanghai:ConnectorStatementScopeTest,SplitProducerTest,ExplainRefreshMTMVCommandTest,StmtExecutorTest,ConnectorSessionImplTest,QueryTaskGroupsTest,QueryFinishCallbackRegistryTest,MysqlConnectProcessorExecuteCleanupTest,ExecuteCommandTest,StatementContextTest,TaskProcessorTest,ConnectProcessorRetryTest,PluginDrivenScanNode*Test(36 classes),PluginDrivenMetadataTest,PluginDrivenMissingTableTest,NereidsPlannerConnectorPlanningTest,SplitAssignmentTest,SplitStreamingConcurrencyTest,CloudInsertProfileRetryTest; fe-core Checkstyle passesCancellationException); a connector session built after that runs without a statement scope.