Doris-Breakwater commented on issue #66796: URL: https://github.com/apache/doris/issues/66796#issuecomment-5310966363
### Breakwater initial analysis **Disposition:** This is a valid BE graceful-shutdown ordering defect on the reported pre-fix `master` revision. It has already been fixed on `master` by merged PR #66797 (commit `b119273e3f06b3425a09908fc0ac65742e6a1b96`), so the issue is correctly closed. **Verified facts** - Before the fix, `doris_main()` stopped and joined the HTTP service before calling `ExecEnv::destroy()`. - `ExecEnv::destroy()` then called `SAFE_STOP(_stream_load_recorder_manager)`, whose `stop()` set `_stop` and synchronously joined its raw `std::thread`. - The worker checked `_stop` only between iterations. Once it entered `_send_stream_load()`, the flag did not interrupt `HttpClient::execute_put_request()`. - That request targets this BE's own HTTP stream-load endpoint and had a 600-second client timeout. An in-flight request that lost its serving HTTP service could therefore keep the join blocked until curl returned. - The reported main-thread stack and shutdown log position are consistent with this path. The source mechanism is confirmed; a worker-thread stack inside curl would have made attribution of the individual CI occurrence fully conclusive, but it is not needed for the current disposition because the defect has been fixed and the fix was merged. **Resolution review** The merged change addresses all relevant race windows: 1. It stops `StreamLoadRecorderManager` before tearing down the BE HTTP service. 2. It prevents the worker from starting another audit stream load after shutdown begins and replaces the fixed one-second sleep with a condition-variable wake-up. 3. It adds an `HttpClient` abort callback backed by libcurl's transfer-progress callback, allowing an already in-flight request to terminate after `_stop` is set rather than waiting for the 600-second timeout. The patch also adds `HttpClientTest.abort_in_flight_request`, which covers interruption of a request whose peer accepts the connection but never responds. Keeping the normal stream-load timeout at 600 seconds is reasonable because shutdown now has a separate cancellation path. **Remaining gap / information needed only if the problem recurs** - There is no deterministic end-to-end test shown for the complete graceful-shutdown sequence with an audit batch in flight; the added unit test validates the critical curl cancellation primitive. - For any recurrence on a build containing `b119273`, collect the exact BE commit/version, the complete shutdown log, an all-thread stack dump (especially the recorder worker), the stream-load audit recorder and batch configuration, and the final curl status/timing. Those data would distinguish this path from another component blocked in `ExecEnv::destroy()`. **Recommended next steps** 1. No additional fix is required for `master` based on the current evidence. 2. Decide whether supported release branches contain the affected recorder implementation; if so, backport #66797 using each branch's actual code history rather than assuming branch/tag naming. 3. If the shutdown test infrastructure permits deterministic synchronization, add an integration test that stops the BE while the audit request is deliberately held in flight and asserts that graceful shutdown remains bounded. Breakwater-GitHub-Analysis-Slot: slot_c3d2749c4d05 -- 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]
