Uh oh!
There was an error while loading. Please reload this page.
[fix](be) Synchronize load error log access - #66250
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#41320 Problem Summary: Load error-log writing and periodic status reporting can concurrently access the same std::ofstream. The reporting path may close and upload the stream while the writer is appending an error row, causing a data race and a BE SIGSEGV in the stream write path. Protect error-log creation, first-error state, stream writes, path retrieval, close, upload, and URL generation with one mutex. Add a fault-injection regression that holds the writer before its first write while periodic reporting requests the error-log path. ### Release note None ### Check List (For Author) - Test: Regression test and local build - BUILD_TYPE=Debug ./build.sh --be -j48 - BUILD_TYPE=Debug ./build.sh --fe --be --cloud -j48 - ./run-regression-test.sh --run -d fault_injection_p0/cloud -s test_load_error_log_concurrent_write -times 3 - Behavior changed: No. This only synchronizes existing load error-log behavior. - Does this need documentation: No
hello-stephen
commented
Jul 30, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
sollhui
commented
Jul 30, 2026
/review |
There was a problem hiding this comment.
Request changes.
The mutex addresses the immediate unsynchronized std::ofstream access, but the end-to-end writer/reporter lifecycle still has three blockers covered inline: periodic upload leaves the active writer closed and silently drops later rows; synchronous S3 transfer now blocks the async writer under the mutex; and the regression does not deterministically prove reporter/writer overlap.
Checkpoint conclusions:
- Goal and proof: the change removes simultaneous stream access, but it does not preserve complete error-log contents across periodic reporting, and the timing-only test can pass without exercising the old race.
- Scope: the diff is small and focused, but the synchronization remedy is incomplete because stream finalization, local-path/URL state, and upload snapshot ownership are not modeled explicitly.
- Concurrency and locks: the competing contexts are the FragmentMgr async result-writer worker and pipeline task-scheduler reporting worker. Shared RuntimeState fields are now guarded and no lock inversion, callback re-entry, or permanent deadlock was found, but remote I/O inside the critical section creates a new writer/backpressure stall.
- Lifecycle, ownership, memory, and errors:
_error_log_fileremains non-null after close, laterofstreamfailures are not checked, and_error_log_file_pathchanges meaning from local path to URL. No new ownership cycle, untracked allocation, cache-handle issue, or static-initialization hazard was found. - Configuration and compatibility: no new config, FE/BE variable, protocol, storage format, persistence, or rolling-upgrade behavior is introduced. The test uses existing mutable BE configs.
- Parallel paths: all append, path, and first-message callers were traced; the issue is common to load sink paths using the per-task RuntimeState, not an isolated caller omission.
- Testing: the cloud regression follows table setup/cleanup conventions, but lacks a two-party synchronization point and post-upload append coverage. Per the review-only runner contract, no build or test was executed during this review.
- Observability, transactions, and data semantics: this change affects diagnostic error-log reporting only; no transaction, visible-version, delete-bitmap, or committed-data path is modified. Existing identifiers/logging are otherwise adequate.
- Performance: S3 upload reads and transfers the whole error file synchronously while holding the new mutex; with the configured limits this is not a bounded lightweight critical section.
- User focus: no additional user-provided focus was supplied; the complete PR was reviewed.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
0AyanamiRei
commented
Aug 3, 2026
run buildall |
hello-stephen
commented
Aug 3, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Aug 3, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
sollhui
commented
Aug 3, 2026
/review |
hello-stephen
commented
Aug 3, 2026
TPC-H: Total hot run time: 28717 ms |
hello-stephen
commented
Aug 3, 2026
TPC-DS: Total hot run time: 169923 ms |
hello-stephen
commented
Aug 3, 2026
ClickBench: Total hot run time: 23.87 s |
There was a problem hiding this comment.
Current-head follow-up: no new unique inline finding remains after full review and deduplication on 6ddee4cd1e4ecb8a7d06ed5b72a01a9749d07278.
The current head addresses the prior remote-I/O critical-section concern: S3 upload and presigning now run after _load_error_log_lock is released, so that thread's writer-blocking premise no longer applies to this revision. Two existing blockers remain and are not duplicated here: periodic/cancellation reporting still closes the non-null active writer and silently loses later rows, and the timing-only regression still does not prove reporter/writer overlap or fail on the unfixed path. Those unresolved threads still prevent approval.
Checkpoint conclusions:
- Goal and proof: the mutex removes simultaneous
ofstreamclose/write access and protects first-error reads, but the end-to-end error-log lifecycle remains incomplete because later writes can target the closed stream. The added test is not a deterministic proof of the fix. - Scope and clarity: the three-file diff is small and focused, but a correct fix still needs an explicit active/finalized or rotating-snapshot state rather than treating close/upload as a terminal transition during periodic reporting.
- Concurrency and locks: scanner/format writer tasks compete with pipeline periodic/cancellation reporting over the per-
RuntimeStatestream and path state. The internal nested order is consistently_s3_error_log_file_lockthen_load_error_log_lock; writers/readers take only the load lock, so no reverse edge or distinct deadlock was found. Concurrent getters are serialized through close/upload/publish. - Lifecycle, ownership, and memory: the copied S3 filesystem handle safely owns the upload dependency and no new ownership cycle, cache-handle leak, untracked long-lived buffer, thread-context issue, or static-initialization hazard was found. The remaining non-null-but-closed stream state is the existing lifecycle blocker.
- Configuration: no configuration item is added. The regression uses existing mutable BE configs before fragment initialization.
- Compatibility: no function/protocol symbol, storage format, FE/BE transport, persistence, or rolling-upgrade contract changes.
- Parallel paths: periodic, cancellation, normal final, stream-load, group-commit, internal-service, and multi-table paths were traced. Periodic/cancellation paths reproduce the existing closed-writer issue; normal final callbacks run after pipeline tasks close. Repeated getter calls are redundant but stable.
- Conditions and error handling: upload
Statusis checked and logged, and first-error access is synchronized. Closed-stream writes still returnOKwithout checking stream state, already covered by the existing lifecycle thread. - Testing and result correctness: the cloud test follows drop-before-use and cleanup conventions, but its sleep has no two-party handshake, the debug point is absent on the base revision, and one invalid row cannot prove post-report durability. No generated result file is involved. Per the review-runner contract, no build or test was executed.
- Observability: existing creation/upload warnings and query/fragment identifiers are adequate for this scoped change; no distinct missing log or metric was substantiated.
- Transactions, persistence, and data semantics: this changes diagnostic error-log handling only; no EditLog, master-failover, visible-version, delete-bitmap, or committed-table-data path is modified. Error-log upload completeness remains governed by the existing lifecycle blocker.
- Performance: remote S3 I/O is now outside
_load_error_log_lock. Formatting and local stream output remain serialized, but no separate correctness or critical-executor failure beyond the existing rotation/snapshot discussion was substantiated. - Additional review: no other correctness, compatibility, memory, observability, or performance issue survived concrete call-chain verification and duplicate suppression.
- User focus: no additional user-provided focus was supplied; the complete PR was reviewed.
### What problem does this PR solve? Issue Number: None Related PR: apache#41320 Problem Summary: Remove the fault-injection regression case and the debug point used only by that case. Keep the production load error-log synchronization change unchanged. ### Release note None ### Check List (For Author) - Test: No need to test (only removes a regression case and its test-only debug point; run buildall will validate the updated PR head) - Behavior changed: No - Does this need documentation: No
0AyanamiRei
commented
Aug 4, 2026
run buildall |
hello-stephen
commented
Aug 4, 2026
TPC-H: Total hot run time: 29038 ms |
hello-stephen
commented
Aug 4, 2026
TPC-DS: Total hot run time: 166681 ms |
hello-stephen
commented
Aug 4, 2026
ClickBench: Total hot run time: 23.83 s |
hello-stephen
commented
Aug 4, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Aug 4, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
sollhui
commented
Aug 13, 2026
/review |
There was a problem hiding this comment.
Request changes.
The current head fixes the prior remote-I/O critical section: S3 upload and presigning now run outside _load_error_log_lock, so that earlier writer-blocking concern no longer applies. Two existing blockers remain and are not duplicated inline here: periodic/cancellation reporting still closes the non-null active writer and silently loses later rows, and there is still no deterministic test proving writer/reporter overlap or failing when the synchronization fix is reverted. This review adds one distinct inline performance finding: all invalid rows after the bounded log reaches its 51-row cap still serialize on the new writer mutex.
Checkpoint conclusions:
- Goal and proof: the mutex removes simultaneous
ofstreamclose/write access and synchronizes the first-error read, but it does not preserve complete error-log contents when periodic or cancellation reporting runs before writers finish. The authoritative diff contains no deterministic regression proof. - Scope and clarity: the two-file diff is small and focused, but the design still conflates an active local writer, a closed upload snapshot, and the published URL instead of modeling the lifecycle explicitly.
- Concurrency and locks: parallel scanner-pool and pipeline workers append while pipeline scheduling/reporting and cancellation paths request the URL. Lock order is consistently
_s3_error_log_file_lockthen_load_error_log_lock; no reverse acquisition or distinct deadlock was found. The new mutex is nevertheless on the per-rejected-row hot path even after no further log write is possible. - Lifecycle, ownership, memory, and errors: the copied S3 filesystem handle safely owns the upload dependency, and no ownership cycle, cache-handle leak, untracked long-lived allocation, thread-context issue, or static-initialization hazard was found. The non-null-but-closed stream still accepts failed insertions without checking stream state and returns
OK, as covered by the existing lifecycle thread. UploadStatusis checked and logged. - Configuration and compatibility: no configuration item, FE/BE variable, protocol, storage format, persistence schema, or rolling-upgrade contract is added or changed.
- Parallel and failure paths: periodic, cancellation, ordinary final, stream-load, group-commit, internal-service, multi-table, repeated-getter, upload-success, and upload-failure paths were traced. Normal final reporting has the task-completion fence; periodic/cancellation paths do not. No additional distinct failure survived duplicate suppression.
- Testing and results: no test or generated result file is changed. The existing fault-injection test uses only a sleep, without a two-party handshake or revert-failure assertion. Per the review-only runner contract, no build or test was executed.
- Observability, transactions, and data semantics: existing creation/upload logging has adequate identifiers for this scoped path. No transaction, EditLog, master-failover, visible-version, delete-bitmap, or committed-table-data behavior is modified; the affected data is diagnostic error-log content.
- Performance: remote S3 I/O is now outside the writer mutex. A high-filter-ratio file can still funnel concurrent scanners through that mutex for every invalid row after the 51-row logging cap, which is the new inline issue.
- Additional review and user focus: all changed lines and callers were swept after two full normal reviews and two risk-focused passes; all subagents converged with no further valuable findings. No additional user-provided focus was supplied, and the complete PR was reviewed.
Uh oh!
There was an error while loading. Please reload this page.
### What problem does this PR solve? Issue Number: None Related PR: apache#41320 Problem Summary: Once load error logging reaches MAX_ERROR_NUM, concurrent scanner workers still acquire _load_error_log_lock before the counter rejects later rows. Add a relaxed atomic pre-check before the mutex and retain the locked fetch_add as the authoritative recheck and slot claim. The fast path preserves the existing error-row limit and zero-tolerance result while avoiding post-cap writer serialization. ### Release note None ### Check List (For Author) - Test: No need to test (no functional behavior change; compilation will be covered by PR buildall) - Behavior changed: No - Does this need documentation: No
d52664c to
3beb646Compare0AyanamiRei
commented
Aug 16, 2026
run buildall |
hello-stephen
commented
Aug 16, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Aug 16, 2026
TPC-H: Total hot run time: 17734 ms |
hello-stephen
commented
Aug 16, 2026
TPC-DS: Total hot run time: 84587 ms |
hello-stephen
commented
Aug 16, 2026
ClickBench: Total hot run time: 14.61 s |
hello-stephen
commented
Aug 16, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Uh oh!
There was an error while loading. Please reload this page.
### What problem does this PR solve? Issue Number: None Related PR: #41320 Load error-log writing and periodic status reporting can concurrently access the same `std::ofstream`. The reporting path may close and upload the stream while the writer is appending an error row, causing a data race and a BE SIGSEGV in the stream write path. This PR uses `_load_error_log_lock` to serialize stream creation and writes and to protect the first-error and path state. A separate `_s3_error_log_file_lock` serializes remote upload and URL publication, while synchronous S3 I/O runs without holding the load-error writer lock.
What problem does this PR solve?
Issue Number: None
Related PR: #41320
Load error-log writing and periodic status reporting can concurrently access the same
std::ofstream. The reporting path may close and upload the stream while the writer is appending an error row, causing a data race and a BE SIGSEGV in the stream write path.This PR uses
_load_error_log_lockto serialize stream creation and writes and to protect the first-error and path state. A separate_s3_error_log_file_lockserializes remote upload and URL publication, while synchronous S3 I/O runs without holding the load-error writer lock.Release note
None
Check List (For Author)