Repository navigation
[fix](be) Reap only the owned CDC client process - #68492
Merged
Merged
Conversation
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.
Static review of exact head e2898117c453b3b1a825649b58bfd846880d38f1 found two test-validity issues, both left inline.
Critical checkpoints:
- Goal/correctness: the production change statically achieves exact-child reaping; the generation-qualified identity and exclusive operation claim prevent unrelated-child reaping and stale-PID operations under the reviewed interleavings.
- Scope/clarity: the change is focused on the CDC manager and its tests, and the ownership state machine is documented.
- Concurrency/lifecycle: both production callers, the single
ExecEnv-owned manager, startup publication, pending handoff, health failure, adoption, stop, and destruction were traced. No concrete double owner, lost reap request, deadlock, or post-release syscall was found. - Error handling/compatibility:
EINTR,ECHILD, early exit, terminal reap, and PID reuse were checked. There is no protocol, storage, persistence, configuration, authorization, or rolling-upgrade contract change. - Parallel paths/performance/observability: both callers share the same manager; spinning is limited to short lifecycle handoffs, and normal-thread logging remains available.
- Tests: the added cases cover unrelated-child isolation, owned reap and
errno, stale generations, handler handoff, and gracefulSIGTERM, but the forced-SIGKILLtest is now vacuous and the stop/handler overlap test is nondeterministic. Please address the inline items. - Validation: static review only; no build or test was run by this reviewer, as required by the automation instructions. Header hygiene and
git diff --checkpassed locally. At submission time compile, clang-format, and checkstyle passed; BE UT was pending. The Cloud P0 check was failing, with no reviewed evidence connecting it to these changes. - User focus: no additional focus was supplied.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28443 ms |
Contributor
TPC-DS: Total hot run time: 152709 ms |
Contributor
ClickBench: Total hot run time: 24 s |
morningman
added a commit
to morningman/doris
that referenced
this pull request
Sep 24, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68492 Problem Summary: The existing forced-kill test supplied a shell background PID, which is not a child of the test process. The new ECHILD guard therefore made stop() return before exercising SIGTERM, SIGKILL, or reaping. Spawn a direct child that reports when it ignores SIGTERM, and verify stop() collects it. The new handler/stop overlap test only waited for 20 ms, so it could pass before the stop thread attempted the ownership claim. Record a failed claim in BE_TEST and require the test to observe it while the handler is paused. ### Release note None ### Check List (For Author) - Test: Manual test: direct child signal behavior, clang-format, check-format, build hygiene, and git diff --check passed. Unit Test: run-be-ut.sh configuration stalled in OpenBLAS getarch before compilation. Clang-tidy: could not complete because the existing libc++ headers fail to parse on this Mac. - Behavior changed: No. Production behavior is unchanged; the claim checkpoint is compiled only for BE_TEST. - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Static review of exact head 13005d6f8b18e61397681fa5bcd6f34c9088bf3b found one test-safety issue, left inline.
Critical checkpoints:
- Goal/correctness: the production change statically achieves exact-child reaping. The generation-qualified identity, exclusive operation claim, and pending-reap handoff prevent unrelated-child reaping, lost notifications, and stale-PID syscalls in the reviewed interleavings.
- Scope/clarity: the production change is focused on
CdcClientMgrand its unit tests. The ownership state machine is complex but locally documented. - Concurrency: the process-wide SIGCHLD handler, RPC/file-reader start calls, health inspection, and stop/destruction paths were traced. Start and stop serialize on
_start_mutex; handler/normal-thread process operations serialize through always-lock-free atomics. No concrete deadlock, double owner, lost reap request, or post-release syscall was found. - Lifecycle/static initialization:
ExecEnvowns one production manager; publication, early child exit, health failure, external adoption, terminal reap, and destruction were checked. Namespace-scope atomics are constant-initialized and the handler retains no manager pointer. - Error handling/compatibility: EINTR, ECHILD, wait errors, early exit, forced termination, and PID reuse were checked. There is no configuration, FE/BE protocol, storage-format, transaction, persistence, authorization, or rolling-upgrade contract change.
- Parallel paths/performance/observability: both production callers share the same manager; the handler does exact-PID WNOHANG work without allocation or logging, and normal-thread lifecycle logs remain available. No distinct performance or observability regression was substantiated.
- Tests: the current head fixes the two prior inline concerns with a direct child that proves SIGTERM is ignored and an observed failed ownership claim. The new stale-generation test still has an unsafe fatal-assertion cleanup order, described inline.
- Validation: static review only; no build or test was run by this reviewer, as required by the automation instructions. The author reports formatting/build-hygiene checks passed, while the targeted BE unit test, full build, and clang-tidy were not completed locally.
- User focus: no additional focus was supplied.
Contributor
TPC-H: Total hot run time: 27583 ms |
Contributor
TPC-DS: Total hot run time: 151727 ms |
Contributor
ClickBench: Total hot run time: 23.91 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
morningman
added a commit
to morningman/doris
that referenced
this pull request
Sep 25, 2026
…ction ### What problem does this PR solve? Issue Number: N/A Related PR: apache#68492 Problem Summary: StaleGenerationCannotTerminateAReusedNumericPid declared its cleanup guard before the manager, and locals are destroyed in reverse declaration order. When one of the test's fatal assertions failed, the manager was destroyed first: its destructor calls stop(), which force-kills and reaps the direct child and thereby frees the numeric pid, and only then did the guard still run kill(pid, SIGKILL) with child_needs_cleanup == true against a pid this process no longer owns. The test's own assertion forbids exactly that - "cleanup retained a stale raw pid and signalled its replacement generation". Declare the manager before the guard so the guard kills and reaps the child while it is still a direct child, and the manager's later stop() observes ECHILD and returns without signalling. ### Release note None ### Check List (For Author) - Test: Manual test: clang-format 16 reported no replacements for the changed file, build-support/check-build-hygiene.sh passed, and the diff is a declaration move plus a comment. The BE unit test was not run locally: configure still stalls in OpenBLAS getarch on this Mac before compiling the changed source. - Behavior changed: No. Test-only reordering; the production change and its semantics are untouched. - Does this need documentation: No Co-Authored-By: Claude Code <[email protected]>
morningman
force-pushed
the
fix-cdc-client-sigchld
branch
from
September 25, 2026 01:06
2c99b98 to
90d1c84
Compare
morningman
added a commit
to morningman/doris
that referenced
this pull request
Sep 25, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68492 Problem Summary: The existing forced-kill test supplied a shell background PID, which is not a child of the test process. The new ECHILD guard therefore made stop() return before exercising SIGTERM, SIGKILL, or reaping. Spawn a direct child that reports when it ignores SIGTERM, and verify stop() collects it. The new handler/stop overlap test only waited for 20 ms, so it could pass before the stop thread attempted the ownership claim. Record a failed claim in BE_TEST and require the test to observe it while the handler is paused. ### Release note None ### Check List (For Author) - Test: Manual test: direct child signal behavior, clang-format, check-format, build hygiene, and git diff --check passed. Unit Test: run-be-ut.sh configuration stalled in OpenBLAS getarch before compilation. Clang-tidy: could not complete because the existing libc++ headers fail to parse on this Mac. - Behavior changed: No. Production behavior is unchanged; the claim checkpoint is compiled only for BE_TEST. - Does this need documentation: No
morningman
added a commit
to morningman/doris
that referenced
this pull request
Sep 25, 2026
…ction ### What problem does this PR solve? Issue Number: N/A Related PR: apache#68492 Problem Summary: StaleGenerationCannotTerminateAReusedNumericPid declared its cleanup guard before the manager, and locals are destroyed in reverse declaration order. When one of the test's fatal assertions failed, the manager was destroyed first: its destructor calls stop(), which force-kills and reaps the direct child and thereby frees the numeric pid, and only then did the guard still run kill(pid, SIGKILL) with child_needs_cleanup == true against a pid this process no longer owns. The test's own assertion forbids exactly that - "cleanup retained a stale raw pid and signalled its replacement generation". Declare the manager before the guard so the guard kills and reaps the child while it is still a direct child, and the manager's later stop() observes ECHILD and returns without signalling. ### Release note None ### Check List (For Author) - Test: Manual test: clang-format 16 reported no replacements for the changed file, build-support/check-build-hygiene.sh passed, and the diff is a declaration move plus a comment. The BE unit test was not run locally: configure still stalls in OpenBLAS getarch on this Mac before compiling the changed source. - Behavior changed: No. Test-only reordering; the production change and its semantics are untouched. - Does this need documentation: No Co-Authored-By: Claude Code <[email protected]>
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of exact head 90d1c8426a17328b9d8fd39c5835780b943a2634 found two additional test-validity/coverage issues. The existing inline threads at cdc_client_mgr.cpp:248 (forced-kill false-positive), cdc_client_mgr_test.cpp:493 (stopper checkpoint), and cdc_client_mgr_test.cpp:533 (fatal cleanup ordering) were treated as hard duplicate fences and are not repeated here.
Critical checkpoints:
- Goal/correctness: the generation-qualified identity, exclusive operation claim, pending-reap handoff, exact-PID waitpid, and ECHILD/ESRCH handling were traced through the signal handler, stop, startup failure, adoption, and ExecEnv destruction paths. No distinct production race or wrong-child operation was substantiated.
- Scope/clarity: the production change is focused on CdcClientMgr ownership and reaping, but the state machine is complex and the production-only startup branches need direct test evidence.
- Concurrency/lifecycle: handler versus normal-thread claim/release interleavings, publication windows, PID reuse, unrelated JVM/Python children, and manager destruction were reviewed. The operation claim remains held through the final process syscall.
- Error handling/compatibility: EINTR, ECHILD, early child exit, forced termination, and external adoption were checked statically; there is no FE/BE protocol, storage, persistence, configuration, or rolling-upgrade contract change.
- Parallel paths/performance/observability: both production callers share the ExecEnv-owned manager; the handler uses exact-PID WNOHANG and lock-free state without allocation/logging. No separate performance or observability defect was found.
- Tests: the two inline findings below remain. Existing force-kill and stopper-barrier comments are already addressed in the current head, while BE_TEST startup tests still bypass the production fork/inspection/adoption path.
- Validation: static review only; no build or test was run by this reviewer, per the review instructions.
- User focus: no additional focus was supplied.
Please address the inline findings and add/update coverage for the production-only lifecycle before merge.
| // rather than behind CdcClientMgr's mutex. The generation prevents a delayed handler from clearing | ||
| // ownership after the kernel has already reused the same numeric pid for a replacement child. | ||
| // ExecEnv owns a single CdcClientMgr, so there is a single published identity. | ||
| static_assert(sizeof(pid_t) <= sizeof(uint32_t)); |
Contributor
There was a problem hiding this comment.
[P2] Exercise the production startup state machine.
All BE unit tests compile with BE_TEST, so this branch returns after publishing synthetic PID 99999 and never executes the newly changed fork/publication window, early-exit reap, health-check failure cleanup, final inspection, or external-adoption logic below. The ownership helper tests cannot prove those end-to-end transitions. Please add a non-BE_TEST/integration case or inject the fork/health outcomes so these production-only paths are actually driven.
Contributor
TPC-H: Total hot run time: 27799 ms |
morningman
added a commit
to morningman/doris
that referenced
this pull request
Sep 25, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68492 Problem Summary: SigchldHandlerDoesNotReapOtherChildren slept a fixed 600 ms and then called blocking waitpid(pid), so it never established that handle_sigchld had executed. On a delayed signal delivery this thread's own waitpid can win the race and reap the unrelated child, and the case then passes without exercising the exact-PID replacement for waitpid(-1) at all. Wait instead for the handler to unpublish the identity it was handed, which it only does after its own waitpid has returned; that is a deterministic handler-run checkpoint, and because it is satisfied before this thread reaps anything, the following reap can no longer overtake the handler. This also removes the last sleep-based synchronization in the file; the other ownership cases already use pause hooks or waitid checkpoints. ### Release note None ### Check List (For Author) - Test: Manual test: a standalone probe that replicates the identity/claim state machine passes the rewritten case 20/20 with the exact-PID waitpid and reports the case is caught when that line is reverted to waitpid(-1). `git diff --check` and `build-support/check-build-hygiene.sh` passed. clang-format 16 is not installed on this Mac, so the changed file was checked by hand against the 100-column limit and the surrounding style. The BE unit test was not run locally: configure still stalls in OpenBLAS getarch on this Mac before compiling the changed source. - Behavior changed: No. Test-only synchronization change; production code is untouched. - Does this need documentation: No Co-Authored-By: Claude Code <[email protected]>
Contributor
TPC-DS: Total hot run time: 153091 ms |
Contributor
ClickBench: Total hot run time: 23.94 s |
morningman
added a commit
to morningman/doris
that referenced
this pull request
Sep 25, 2026
… preconditions ### What problem does this PR solve? Issue Number: N/A Related PR: apache#68492 Problem Summary: An external review of this PR found that the post-fork early return was placed before the health check, so a forked child that died inside the fork-to-inspection window was reported as a startup error even when the port had come back under an external cdc client. The base ran the health check unconditionally and adopted in exactly that case. Move the decision after the health check: the inspection still reaps the child and revokes its identity, and a failing health check reports the early exit as before. Because the identity is already unpublished by then, the existing final inspection returns NOT_OWNED, which is the branch that reaches adoption, so no new outcome had to be written. The price of restoring base ordering is that an immediately dead child is now reported after the health-check window instead of within microseconds. Three further findings are fixed. claim_child_identity() asserts a non-zero identity: identity 0 names no generation, so its loop could never observe a change and would spin forever while stop() holds _start_mutex, instead of reporting the impossible input. terminate_and_reap_child() and _publish_child_pid() assert their pid precondition the same way rather than absorbing it with a silent return. The sigaction() install result is checked, because nothing else consumes the published identity and a silently failed install would disable the pending-reap handoff. On the test side, StopWithRealProcessGraceful is renamed to StopDoesNotSignalANonChildPid: its published pid is a popen grandchild this process can never reap, so it exercises the ECHILD early return rather than a graceful shutdown, and it now asserts the pid is still alive afterwards, which is the property that early return exists for. StaleGenerationCannotTerminateAReusedNumericPid gives its child a SIGTERM trap that reports on a pipe, so the graceful half of the reap sequence is observable instead of being indistinguishable from the forced kill. Two post-join flag assertions that could not fail are removed. The review also found that a failed health check now terminates and reaps the child this process forked, where the base dropped the PID and left the child running unmanaged. Removing the base's accidental recovery - a later attempt adopting the still-warming JVM - is intended, and is now declared in the PR body and the release note. The reaping rule itself is not altered. ### Release note See the PR body: a startup that fails its health check now terminates and reaps the CDC client it forked instead of leaving it running unmanaged. ### Check List (For Author) - Test: Manual test. A standalone probe reproducing the claim/inspect helpers confirms that an initial inspection observing EXITED leaves the identity unpublished and the second inspection returns NOT_OWNED, which is the adoption branch. The rewritten test child reports SIGTERM on the pipe within the grace window in 5/5 runs, and a SIGKILL shows up as EOF rather than a blocking read. `git diff --check` and `build-support/check-build-hygiene.sh` passed. clang-format 16 is not installed on this Mac, so both files were checked by hand against the 100-column limit and the surrounding style. The BE unit test was not run locally: configure still stalls in OpenBLAS getarch on this Mac. - Behavior changed: Yes for the reaping rule and the failed-startup cleanup, both declared in the PR body; the adoption-window change restores base behaviour. - Does this need documentation: No Co-Authored-By: Claude Code <[email protected]>
morningman
added a commit
to morningman/doris
that referenced
this pull request
Sep 25, 2026
… preconditions ### What problem does this PR solve? Issue Number: N/A Related PR: apache#68492 Problem Summary: An external review of this PR found that the post-fork early return was placed before the health check, so a forked child that died inside the fork-to-inspection window was reported as a startup error even when the port had come back under an external cdc client. The base ran the health check unconditionally and adopted in exactly that case. Move the decision after the health check: the inspection still reaps the child and revokes its identity, and a failing health check reports the early exit as before. Because the identity is already unpublished by then, the existing final inspection returns NOT_OWNED, which is the branch that reaches adoption, so no new outcome had to be written. The price of restoring base ordering is that an immediately dead child is now reported after the health-check window instead of within microseconds. Three further findings are fixed. claim_child_identity() asserts a non-zero identity: identity 0 names no generation, so its loop could never observe a change and would spin forever while stop() holds _start_mutex, instead of reporting the impossible input. terminate_and_reap_child() and _publish_child_pid() assert their pid precondition the same way rather than absorbing it with a silent return. The sigaction() install result is checked, because nothing else consumes the published identity and a silently failed install would disable the pending-reap handoff. On the test side, StopWithRealProcessGraceful is renamed to StopDoesNotSignalANonChildPid: its published pid is a popen grandchild this process can never reap, so it exercises the ECHILD early return rather than a graceful shutdown, and it now asserts the pid is still alive afterwards, which is the property that early return exists for. StaleGenerationCannotTerminateAReusedNumericPid gives its child a SIGTERM trap that reports on a pipe, so the graceful half of the reap sequence is observable instead of being indistinguishable from the forced kill. Two post-join flag assertions that could not fail are removed. The review also found that a failed health check now terminates and reaps the child this process forked, where the base dropped the PID and left the child running unmanaged. Removing the base's accidental recovery - a later attempt adopting the still-warming JVM - is intended, and is now declared in the PR body and the release note. The reaping rule itself is not altered. ### Release note See the PR body: a startup that fails its health check now terminates and reaps the CDC client it forked instead of leaving it running unmanaged. ### Check List (For Author) - Test: Manual test. A standalone probe reproducing the claim/inspect helpers confirms that an initial inspection observing EXITED leaves the identity unpublished and the second inspection returns NOT_OWNED, which is the adoption branch. The rewritten test child reports SIGTERM on the pipe within the grace window in 5/5 runs, and a SIGKILL shows up as EOF rather than a blocking read. `git diff --check` and `build-support/check-build-hygiene.sh` passed. clang-format 16.0.6 (Homebrew llvm@16) reports no replacements for the three changed files. The BE unit test was not run locally: configure still stalls in OpenBLAS getarch on this Mac. - Behavior changed: Yes for the reaping rule and the failed-startup cleanup, both declared in the PR body; the adoption-window change restores base behaviour. - Does this need documentation: No Co-Authored-By: Claude Code <[email protected]>
morningman
force-pushed
the
fix-cdc-client-sigchld
branch
from
September 25, 2026 02:19
88cf0e0 to
7ad6a00
Compare
Contributor
There was a problem hiding this comment.
Static review of exact head e1699c514239b2168d5fdee02ae7a1675cc56ea5 found two additional liveness issues, both left inline. The nine existing threads were treated as hard duplicate fences and were not repeated.
Critical checkpoints:
- Goal/correctness: the generation-qualified exact-PID state machine otherwise prevents unrelated-child reaping and stale-PID operations in the reviewed interleavings, but failed-start cleanup can monopolize the CDC lifecycle indefinitely after
SIGKILL. - Scope/clarity: production changes are localized to
CdcClientMgr; the header adds internal lifecycle helpers, and the unrelated Decimal64 test casts preserve the exactInt64values. - Concurrency/lifecycle/static initialization: normal start/stop calls serialize on
_start_mutex; the signal path uses constant-initialized, always-lock-free atomics and exact-generation ownership. Pending-reap, stale-generation retry, PID reuse, publication, adoption, health failure, stop, andExecEnvdestruction were traced. No separate lost handoff, double owner, static-order hazard, or post-release PID syscall was found. - Error handling/observability/performance:
EINTR,ECHILD, early exit, graceful termination, forced termination, and cleanup were checked. Normal lifecycle logging remains available and the handler stays allocation- and lock-free. The unbounded post-kill wait is the remaining production error-path defect. - Parallel paths/compatibility: both the RPC heavy-work path and
HttpFileReadershare the singleton manager; other child owners keep exact-PID waits. There is no new configuration, FE/BE protocol, storage format, persistence, transaction, data-write, authorization, glibc-baseline, or rolling-upgrade contract. - Tests: the added cases broadly cover ownership and interleavings, but three handler-contention checks can hang the shared BE test binary instead of failing. The production-only startup coverage gap and other test defects already have existing inline threads.
- Validation: static review only. Per the review instructions, no build or test was run, and author/CI claims are not independent validation by this reviewer.
git diff --checkpassed for the exact base/head pair. - Completion: three independent normal/risk-focused rounds converged with no unresolved candidate beyond the two inline findings.
- User focus: no additional focus was supplied.
| << ", error=" << strerror(errno); | ||
| } | ||
| do { | ||
| wait_result = waitpid(pid, &status, 0); |
Contributor
There was a problem hiding this comment.
[P2] Keep failed-start cleanup bounded after SIGKILL
The health-check failure path now reaches this helper while start_cdc_client() still holds _start_mutex, but after SIGKILL this blocking waitpid(..., 0) has no deadline. A task stuck in uninterruptible I/O can remain un-waitable despite the pending kill, leaving the request, the operation token, every later start/stop, and eventual manager destruction blocked indefinitely. Doris already handles this condition in PythonUDFProcess::shutdown() with a bounded post-kill wait and a background reaper. Please preserve the exact-generation ownership guarantee by transferring an unreaped child to a unique per-PID asynchronous owner before releasing the foreground claim, so failed startup can return without blocking successor publication.
|
|
||
| // This handler sees the successor, but the stale claim prevents it from recording a pending | ||
| // reap. The delayed handler must take responsibility after releasing that stale claim. | ||
| CdcClientMgr::invoke_sigchld_handler_for_test(); |
Contributor
There was a problem hiding this comment.
[P2] Bound the handler-contention checks
This synchronous invocation runs while the delayed handler deliberately holds the stale generation's operation token, and that owner is not released until line 844. If the foreign-token path regresses to waiting for the token, this call never returns and its only release is unreachable, hanging the whole shared BE test binary instead of failing the case. The same circular harness dependency occurs at second.join() around line 884 and the synchronous invocation around line 950. Please run each challenged handler asynchronously, observe completion with a bounded deadline, then always release/join the owner before failing (or isolate these liveness checks in a subprocess with a timeout).
Contributor
TPC-DS: Total hot run time: 152655 ms |
Contributor
ClickBench: Total hot run time: 23.96 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: N/A Related PR: apache#66399 Problem Summary: BE's process-wide SIGCHLD handler called waitpid(-1), which could reap a child launched by the embedded JVM. The JVM could then report a failed child process as successful. This change scopes reaping to the CDC client and uses a generation-qualified child identity plus an exclusive operation claim to prevent the handler, startup, and shutdown paths from acting on a reused PID. The three CDC manager files are extracted unchanged from apache#66399. ### Release note Fix CDC client lifecycle handling so BE preserves the exit status of unrelated child processes. ### Check List (For Author) - Test: Manual test: clang-format, check-format, build hygiene, git diff --check, and exact comparison with apache#66399 passed. Unit Test: CdcClientMgrTest was not run because the local thirdparty installation lacks libsimdutf.a. The ASAN BE build was stopped after CLucene CMake try-run probes hung on this Mac. Clang-tidy could not complete because the existing Release compilation database produced libc++ system-header errors. - Behavior changed: Yes. SIGCHLD reaps only the owned CDC child; startup and shutdown guard process operations against PID reuse. - Does this need documentation: No.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68492 Problem Summary: The existing forced-kill test supplied a shell background PID, which is not a child of the test process. The new ECHILD guard therefore made stop() return before exercising SIGTERM, SIGKILL, or reaping. Spawn a direct child that reports when it ignores SIGTERM, and verify stop() collects it. The new handler/stop overlap test only waited for 20 ms, so it could pass before the stop thread attempted the ownership claim. Record a failed claim in BE_TEST and require the test to observe it while the handler is paused. ### Release note None ### Check List (For Author) - Test: Manual test: direct child signal behavior, clang-format, check-format, build hygiene, and git diff --check passed. Unit Test: run-be-ut.sh configuration stalled in OpenBLAS getarch before compilation. Clang-tidy: could not complete because the existing libc++ headers fail to parse on this Mac. - Behavior changed: No. Production behavior is unchanged; the claim checkpoint is compiled only for BE_TEST. - Does this need documentation: No
…ction ### What problem does this PR solve? Issue Number: N/A Related PR: apache#68492 Problem Summary: StaleGenerationCannotTerminateAReusedNumericPid declared its cleanup guard before the manager, and locals are destroyed in reverse declaration order. When one of the test's fatal assertions failed, the manager was destroyed first: its destructor calls stop(), which force-kills and reaps the direct child and thereby frees the numeric pid, and only then did the guard still run kill(pid, SIGKILL) with child_needs_cleanup == true against a pid this process no longer owns. The test's own assertion forbids exactly that - "cleanup retained a stale raw pid and signalled its replacement generation". Declare the manager before the guard so the guard kills and reaps the child while it is still a direct child, and the manager's later stop() observes ECHILD and returns without signalling. ### Release note None ### Check List (For Author) - Test: Manual test: clang-format 16 reported no replacements for the changed file, build-support/check-build-hygiene.sh passed, and the diff is a declaration move plus a comment. The BE unit test was not run locally: configure still stalls in OpenBLAS getarch on this Mac before compiling the changed source. - Behavior changed: No. Test-only reordering; the production change and its semantics are untouched. - Does this need documentation: No Co-Authored-By: Claude Code <[email protected]>
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68492 Problem Summary: SigchldHandlerDoesNotReapOtherChildren slept a fixed 600 ms and then called blocking waitpid(pid), so it never established that handle_sigchld had executed. On a delayed signal delivery this thread's own waitpid can win the race and reap the unrelated child, and the case then passes without exercising the exact-PID replacement for waitpid(-1) at all. Wait instead for the handler to unpublish the identity it was handed, which it only does after its own waitpid has returned; that is a deterministic handler-run checkpoint, and because it is satisfied before this thread reaps anything, the following reap can no longer overtake the handler. This also removes the last sleep-based synchronization in the file; the other ownership cases already use pause hooks or waitid checkpoints. ### Release note None ### Check List (For Author) - Test: Manual test: a standalone probe that replicates the identity/claim state machine passes the rewritten case 20/20 with the exact-PID waitpid and reports the case is caught when that line is reverted to waitpid(-1). `git diff --check` and `build-support/check-build-hygiene.sh` passed. clang-format 16 is not installed on this Mac, so the changed file was checked by hand against the 100-column limit and the surrounding style. The BE unit test was not run locally: configure still stalls in OpenBLAS getarch on this Mac before compiling the changed source. - Behavior changed: No. Test-only synchronization change; production code is untouched. - Does this need documentation: No Co-Authored-By: Claude Code <[email protected]>
… preconditions ### What problem does this PR solve? Issue Number: N/A Related PR: apache#68492 Problem Summary: An external review of this PR found that the post-fork early return was placed before the health check, so a forked child that died inside the fork-to-inspection window was reported as a startup error even when the port had come back under an external cdc client. The base ran the health check unconditionally and adopted in exactly that case. Move the decision after the health check: the inspection still reaps the child and revokes its identity, and a failing health check reports the early exit as before. Because the identity is already unpublished by then, the existing final inspection returns NOT_OWNED, which is the branch that reaches adoption, so no new outcome had to be written. The price of restoring base ordering is that an immediately dead child is now reported after the health-check window instead of within microseconds. Three further findings are fixed. claim_child_identity() asserts a non-zero identity: identity 0 names no generation, so its loop could never observe a change and would spin forever while stop() holds _start_mutex, instead of reporting the impossible input. terminate_and_reap_child() and _publish_child_pid() assert their pid precondition the same way rather than absorbing it with a silent return. The sigaction() install result is checked, because nothing else consumes the published identity and a silently failed install would disable the pending-reap handoff. On the test side, StopWithRealProcessGraceful is renamed to StopDoesNotSignalANonChildPid: its published pid is a popen grandchild this process can never reap, so it exercises the ECHILD early return rather than a graceful shutdown, and it now asserts the pid is still alive afterwards, which is the property that early return exists for. StaleGenerationCannotTerminateAReusedNumericPid gives its child a SIGTERM trap that reports on a pipe, so the graceful half of the reap sequence is observable instead of being indistinguishable from the forced kill. Two post-join flag assertions that could not fail are removed. The review also found that a failed health check now terminates and reaps the child this process forked, where the base dropped the PID and left the child running unmanaged. Removing the base's accidental recovery - a later attempt adopting the still-warming JVM - is intended, and is now declared in the PR body and the release note. The reaping rule itself is not altered. ### Release note See the PR body: a startup that fails its health check now terminates and reaps the CDC client it forked instead of leaving it running unmanaged. ### Check List (For Author) - Test: Manual test. A standalone probe reproducing the claim/inspect helpers confirms that an initial inspection observing EXITED leaves the identity unpublished and the second inspection returns NOT_OWNED, which is the adoption branch. The rewritten test child reports SIGTERM on the pipe within the grace window in 5/5 runs, and a SIGKILL shows up as EOF rather than a blocking read. `git diff --check` and `build-support/check-build-hygiene.sh` passed. clang-format 16.0.6 (Homebrew llvm@16) reports no replacements for the three changed files. The BE unit test was not run locally: configure still stalls in OpenBLAS getarch on this Mac. - Behavior changed: Yes for the reaping rule and the failed-startup cleanup, both declared in the PR body; the adoption-window change restores base behaviour. - Does this need documentation: No Co-Authored-By: Claude Code <[email protected]>
…nal environment ### What problem does this PR solve? Issue Number: N/A Related PR: apache#68492 Problem Summary: BE UT build 84422 failed CdcClientMgrTest.StaleGenerationCannotTerminateAReusedNumericPid: after stop() returned, the report pipe was at EOF instead of holding the child's 'T', and the test took 1026 ms where its neighbours in the same suite take ~600 ms - the 200 ms grace in terminate_and_reap_child expired and the forced kill collected the child. The child's trap is the witness for the graceful half of the reap, and it was spawned with posix_spawn(..., &actions, nullptr, ...), so it inherited whatever process-wide signal state doris_be_test was in. Every suite in that binary runs in one process, so 13k tests' worth of state precedes this one, and when SIGTERM is ignored or blocked there the witness cannot fire at all: a non-interactive shell refuses to trap a signal that was ignored on entry, silently - `trap -p TERM` stays empty and nothing is printed - and a blocked signal is never delivered. The forced kill also leaves the pipe at EOF, so the failure reads as "stop() never delivered SIGTERM", which is what the first assertion's message claimed. Reproduced on this machine with the real test binary: with the pre-fix test body and SIGTERM ignored in the test process, the case fails at cdc_client_mgr_test.cpp:601 and :603 - the same lines and the same shape the CI reported, 256 ms against 26 ms when it passes. The same reproduction was also done with a standalone program that posix_spawns the same child, which additionally shows that bash 3.2, 4.2 and 5.3 all run the trap in under a millisecond when the parent is clean, so the shell is not where the difference lives. The child therefore gets POSIX_SPAWN_SETSIGDEF (SIGTERM and SIGCHLD to SIG_DFL, which also keeps its own `wait` off an inherited SIGCHLD=SIG_IGN - the case python_env_test.cpp resets by hand) and POSIX_SPAWN_SETSIGMASK with an empty set. This does not narrow what the test exercises: terminate_and_reap_child still signals a child that starts with the default disposition and an empty mask, which is what a freshly exec'd child has in a deployment. The first assertion's message now reports that the child was collected without reporting the trap, instead of asserting that the signal was never sent. ### Release note None ### Check List (For Author) - Test: Manual test, on the real binary. Built with BUILD_TYPE_UT=Debug (ASAN deadlocks at startup on this Mac) and ran three ways: (1) the whole suite as run-be-ut.sh builds and runs it - 35/35 passed, this case at 29 ms; (2) the same binary with SIGTERM ignored in the test process, which is the CI's inherited state - passed at 26 ms; (3) the same binary with the pre-fix test body in that same CI-like environment - failed at cdc_client_mgr_test.cpp:601 and :603, reproducing the CI failure. A standalone posix_spawn probe additionally covered the blocked-mask variant of the same state and the shell versions. build-support/check-build-hygiene.sh passes; clang-format 16.0.6 (Homebrew llvm@16) reports no replacements for the changed file. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Code <[email protected]>
### What problem does this PR solve? Issue Number: N/A Related PR: N/A Problem Summary: A SIGCHLD handler can read CDC child generation N before claiming its operation token. If N is reaped and generation N+1 is published first, the delayed handler can claim stale N. The N+1 handler then drops its only exit notification behind that stale claim, leaving the child published as a zombie. Recheck the current generation after a stale claim is released so the delayed handler collects the successor. Add a deterministic cross-generation test. Strengthen the non-child, unrelated-child cleanup, and stale-PID tests so their assertions observe the intended signals and release test children on failure. ### Release note Prevent an exited CDC client from remaining published as a zombie when an older SIGCHLD handler races with a replacement child. ### Check List (For Author) - Test: Unit Test: BUILD_TYPE_UT=Debug ./run-be-ut.sh --run --filter='CdcClientMgrTest.*:FormatRoundTest.*' -j 8 (44/44 passed, including 36 CDC tests; temporary local exclusions for unrelated macOS-incompatible tests were reverted before commit). Manual test: clang-format, check-format, build hygiene, and git diff --check passed. Clang-tidy was attempted but did not complete because of libc++ system-header parse errors on this Mac. - Behavior changed: Yes. A delayed handler now rechecks the current CDC generation after releasing a stale claim and can reap its exited successor. - Does this need documentation: No
morningman
force-pushed
the
fix-cdc-client-sigchld
branch
from
September 28, 2026 02:03
e1699c5 to
0608d7a
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28024 ms |
Contributor
TPC-DS: Total hot run time: 152577 ms |
Contributor
ClickBench: Total hot run time: 24.04 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: {"type":"error","status":400,"error":{"type":"invalid_request_error","message":"The 'gpt-6-sol' model is not supported when using Codex with a ChatGPT account."}} Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Static review of exact head 0608d7a585c7c523e0243deff76e215cfb403574 found no new distinct inline findings. I request changes because three concerns remain in the current code and are already covered by inline threads: the unbounded wait after SIGKILL, the synchronous handler-contention probes that can hang the shared test binary, and the production startup branches bypassed by BE_TEST. I am not reposting duplicate inline comments.
Critical checkpoints:
- Goal and correctness: Exact-PID
waitpidavoids reaping unrelated BE children. The generation-qualified operation claim, pending-reap bit, and stale-handler retry close the reviewed same- and cross-generation handoffs. Failed-start cleanup can still block the CDC lifecycle indefinitely at the already-threaded post-SIGKILL wait. - Scope and clarity: The change is confined to the CDC manager, its header, and tests. The ownership state machine is complex but documented near its operations.
- Concurrency and lifecycle: The process-wide SIGCHLD handler, RPC and
HttpFileReadercallers, singletonExecEnvmanager, fork/publication, health inspection, adoption, stop, and destruction were traced. Start and stop serialize on_start_mutex; the handler uses lock-free atomics and exact-generation claims. No additional lost notification, double owner, stale-PID syscall, lock-order defect, or static-initialization hazard was substantiated. - Error handling:
EINTR,ECHILD, early exit, and ownership revocation were checked. The unbounded wait after a failed-start SIGKILL remains a concrete error-path defect. - Memory and resource safety: The handler allocates nothing, the new atomics require no dynamic initialization, and exact-PID collection is intended to release child-process resources. No new cache, tracked-buffer, or ownership-cycle path is introduced.
- Parallel paths, compatibility, and configuration: Both CDC callers use the same manager. No new configuration, FE/BE variable, protocol, storage format, transaction, persistence, data-write, or rolling-upgrade contract is introduced. No separate performance or observability defect was substantiated; normal-thread lifecycle logs remain and the handler does no allocation or logging.
- Tests: Direct-child force kill, unrelated-child isolation, stale generations, and pending-reap handoff have BE unit cases. The production-only fork/health/adoption path has no direct test, and three contention checks can hang rather than fail. The relevant details are already inline.
- Validation: Static inspection only, as required for this review runner. I did not build, run tests, or treat author/CI claims as independent validation.
- User focus: No additional focus was supplied.
The single main risk scan, both normal full reviews, and the risk-focused review converged with no new valuable findings; every candidate was checked against the existing threads.
3 of 8 tasks
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: N/A
Problem Summary:
When BE starts the CDC client,
CdcClientMgrinstalls a process-wide SIGCHLD handler. The handler previously calledwaitpid(-1, ..., WNOHANG), which could reap any child of BE. The embedded JVM also starts subprocesses and waits for their exit codes. If the CDC handler reaps one first, the JVM can observeECHILDand report exit code 0 even when the command failed. One observed result was a failed libc-detection probe appearing successful, causing RocksDB JNI to select the musl library on a glibc host.The handler now waits only for the CDC child. A generation-qualified identity and an exclusive operation claim coordinate it with startup, health inspection, and shutdown, so those paths cannot act on a PID after it has been reaped and reused. A pending-reap handoff preserves SIGCHLD notifications arriving during inspection. The latest review identified another handoff window: an old handler could read generation N before claiming, then claim N after N+1 was published and hide N+1's only exit notification. The handler now rechecks the published generation after releasing that stale claim. A deterministic test pauses at both sides of this window and verifies that N+1 is reaped.
The affected tests now fail if their process setup fails, observe SIGTERM through child-reported markers and phase handshakes instead of
kill(pid, 0), and reap an unrelated direct child even when a fatal handler checkpoint fails. The forced-shutdown test uses a direct child that ignores SIGTERM; the handler/shutdown test waits for an actual failed ownership claim; and the stale-generation helper starts with a controlled signal environment.The ownership rule changes what a failed startup does to its forked child. The health-check failure path now terminates and reaps it, where the PID used to be dropped and the child left running unmanaged. That abandoned child could finish warming up and be adopted by a later
start_cdc_clientcall. On a host whose coldcdc-client.jarJVM consistently answers/actuator/healthlater than the unchanged startup window, pre-start the CDC client so the adoption path finds it, or give the JVM more room. The cleanup also removes the unmanaged JVM left holdingcdc_client_port.The macOS BE test compatibility changes, including the Decimal64 literal fix, were merged separately in #68520. This branch was rebased onto that merge and its duplicate test-only commit was dropped.
Release note
Fix CDC client lifecycle handling so BE preserves the exit status of unrelated child processes and reaps an exited replacement CDC client even if an older SIGCHLD handler resumes late. A startup that fails its health check now terminates and reaps the client it forked instead of leaving it running unmanaged; on a host where that JVM reliably needs longer than the startup health-check window, start it before BE so the adoption path finds it.
Check List (For Author)
Test
BUILD_TYPE_UT=Debug ./run-be-ut.sh --run --filter='CdcClientMgrTest.*:FormatRoundTest.*' -j 8passed locally: 44/44 tests (36 CDC manager, 8 FormatRound). Debug was used because ASAN test startup hangs on this macOS 26.5 machine. To build the UT binary locally, I temporarily excluded unrelated SNII and segment tests that fail to compile or link on this Mac and supplied a worktree-local Faiss include link; the CMake exclusion was reverted before commit. TheBE_TESTfast path does not exercise production-only fork, health-check, and adoption orchestration; that remains an integration-test follow-up.build-support/clang-format.sh,build-support/check-format.sh,build-support/check-build-hygiene.sh, andgit diff --checkpassed.try_runprobes hung on this Mac, before compiling the changed source.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)