Doris-Breakwater commented on issue #68169: URL: https://github.com/apache/doris/issues/68169#issuecomment-5726966337
Breakwater-GitHub-Analysis-Slot: slot_e692bb511ebb ## Initial assessment **This is a valid FE connection-teardown/cancellation gap, but the reported “indefinite query after `query_timeout`” mechanism is only partially established.** A disconnected MySQL session can leave its active query running until an execution-side timeout or another terminal event. That is enough to retain a workload-group slot unnecessarily and is worth fixing. However, static inspection does not support the stronger conclusion that removal from `connectionMap` by itself bypasses every query timeout. I checked the current heads named by the report: `master` at [`44e3ae2b`](https://github.com/apache/doris/commit/44e3ae2b9518e1f11595259136ecd3e5d2d3555a), `branch-4.1` at [`8d2018bd`](https://github.com/apache/doris/commit/8d2018bdc72c989d885814215644294478041aa5), and `branch-3.0` at [`4090f6c4`](https://github.com/apache/doris/commit/4090f6c40d2238b7aff73185bcada48f96fbd6be). The relevant teardown structure is present in all three. ### Confirmed from code - The close listener calls only `unregisterConnection(context)` ([`AcceptListener`](https://github.com/apache/doris/blob/44e3ae2b9518e1f11595259136ecd3e5d2d3555a/fe/fe-core/src/main/java/org/apache/doris/mysql/AcceptListener.java#L112-L116)). - `ConnectPoolMgr.timeoutChecker()` iterates only `connectionMap.values()`, while `unregisterConnection()` closes a transaction and removes the context without cancelling its active executor ([timeout scan and unregister](https://github.com/apache/doris/blob/44e3ae2b9518e1f11595259136ecd3e5d2d3555a/fe/fe-core/src/main/java/org/apache/doris/qe/ConnectPoolMgr.java#L54-L97)). Therefore, after the close callback wins the race, that context no longer receives `ConnectContext.checkTimeout()` calls. This part of the report is correct. - For a normal coordinator query, cancellation already has the required downstream behavior: `Coordinator.cancel()` cancels the queue token and invokes asynchronous BE-fragment cancellation ([source](https://github.com/apache/doris/blob/44e3ae2b9518e1f11595259136ecd3e5d2d3555a/fe/fe-core/src/main/java/org/apache/doris/qe/Coordinator.java#L1407-L1437), [RPC path](https://github.com/apache/doris/blob/44e3ae2b9518e1f11595259136ecd3e5d2d3555a/fe/fe-core/src/main/java/org/apache/doris/qe/Coordinator.java#L1459-L1472)). When the worker unwinds, `Coordinator.close()` releases the queue token ([source](https://github.com/apache/doris/blob/44e3ae2b9518e1f11595259136ecd3e5d2d3555a/fe/fe-core/src/main/java/org/apache/doris/qe/Coordinator.java#L818-L827)), and normal query finalization removes the query from `QeProcessorImpl` ([source](https://github.com/apache/doris/blob/44e3ae2b9518e1f11595259136ecd3e5d2d3555a/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java#L1125-L1131)). The missing link is invoking cancellation when the transport closes. - Until that happens, no write is required while `coordBase.getNext()` waits, so losing the client socket need not produce an `EPIPE` that wakes the worker. The queue token remains held until the query exits and the `finally` block closes the coordinator ([source](https://github.com/apache/doris/blob/44e3ae2b9518e1f11595259136ecd3e5d2d3555a/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java#L1690-L1728), [cleanup](https://github.com/apache/doris/blob/44e3ae2b9518e1f11595259136ecd3e5d2d3555a/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java#L1772-L1800)). ### Important qualification The ordinary BE-result path also enforces an absolute execution deadline independently of `TimeoutChecker`: `Coordinator` derives `timeoutDeadline` from `queryOptions.execution_timeout` and gives it to each `ResultReceiver` ([source](https://github.com/apache/doris/blob/44e3ae2b9518e1f11595259136ecd3e5d2d3555a/fe/fe-core/src/main/java/org/apache/doris/qe/Coordinator.java#L854-L890)); `ResultReceiver.getNext()` uses that deadline in its timed future wait and returns a timeout status ([source](https://github.com/apache/doris/blob/44e3ae2b9518e1f11595259136ecd3e5d2d3555a/fe/fe-core/src/main/java/org/apache/doris/qe/ResultReceiver.java#L98-L132)). Waiting for a workload-group token is likewise bounded by the smaller applicable queue/execution timeout ([source](https://github.com/apache/doris/blob/44e3ae2b9518e1f11595259136ecd3e5d2d3555a/fe/fe-core/src/main/java/org/apache/doris/resource/workloadgroup/QueueToken.java#L87-L103)). Consequently, “the context was removed from `connectionMap`, therefore this `getNext()` can run forever past its effective execution timeout” is not yet proven. The observed >3,400-second lifetime may reveal another blocked path or timeout discrepancy, but it needs runtime evidence. Also, although the production SQL is described as a point lookup, the short-circuit `PointQueryExecutor` branch does not register a coordinator in `QeProcessorImpl` ([source](https://github.com/apache/doris/blob/44e3ae2b9518e1f11595259136ecd3e5d2d3555a/fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java#L1625-L1650)); that path cannot directly explain the reported `active_queries` row plus workload-group token. An `EXPLAIN`/profile is needed to identify the actual execution path. ### Information needed to confirm the long-lived symptom Please add: 1. Exact FE and BE version strings/commit IDs for one occurrence, including whether the cluster was mixed-version. 2. The query ID and timestamped `active_queries` snapshots, plus the effective session/user `query_timeout`, any `SET_VAR` override, and the profile/query-options execution timeout for that same query. 3. FE logs for that query ID from before disconnect through the point where it exceeded the expected timeout, especially `get result timeout`, `kill query timeout`, `Cancel execution`, and deregistration messages. 4. An FE thread dump while the query is already past its effective timeout. The exact blocking frame is necessary; stating `coordBase.getNext()` alone does not distinguish the receiver, short-circuit, or another coordinator path. 5. A deterministic reproducer for the BE wait (including table model and the operation holding the rowset/delete-bitmap lock), the client/driver and close method, and `EXPLAIN` or the query profile. The current “slow query” step proves prompt disconnect cancellation only if the blocking condition is repeatable. ### Recommended next steps 1. Treat connection teardown as a cancellation source: cancel the active execution with a specific client-disconnected `CANCELLED` reason **before** removing/releasing session state. Keep this idempotent and race-safe. Avoid directly removing the query from `QeProcessorImpl` in the close callback; cancellation should wake the execution thread, whose existing `finally` path owns coordinator close, queue-token release, profile finalization, and query deregistration. 2. Do not blindly block the XNIO close-listener thread. `ConnectContext.cancelQuery()` reaches `StmtExecutor.cancel()`, which may wait for a `CancelableCommand`; the implementation should preserve prompt asynchronous transport teardown while still propagating coordinator/BE cancellation. 3. Coordinate the patch with open PR [#68101](https://github.com/apache/doris/pull/68101), which is actively changing `ConnectPoolMgr.unregisterConnection()`. Its current MySQL session-release hook is intentionally a no-op, so that refactor does not by itself close this gap. 4. Add a focused unit test that unregistering a context with an active executor dispatches cancellation exactly once, plus an integration/regression test with `max_concurrency=1` that closes the client during a deterministic BE wait and asserts: the first query is cancelled, the queue token is released, the `active_queries` row disappears, and a second query starts without waiting for queue timeout. Cover both FIN and RST/dropped-client behavior and the race with normal query completion. Triage metadata: the issue currently has no labels and no assignee; the author is willing to submit a PR. Applying the repository’s FE/query-execution/workload-group bug ownership labels would help route it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
