Repository navigation
Judge the race step by what the detector found - #50
Merged
Merged
Conversation
main has been red since #45 on one step, and not for anything in this repository. `go test -race` reports every test as PASS and then the process segfaults on the way out, so the step fails on an exit status that says nothing about the tests. From a core dump: the crashing thread's stack is runtime.raise <- raisebadsignal <- badsignal <- sigtrampgo, which is Go's path for a signal on a thread it does not own — a C thread. __tsan_fini and __run_exit_handlers are on the stack, and 33 threads are parked in __pthread_cond_wait_common inside libchdb. So: the engine's process-global ClickHouse pools are still alive at exit because nothing can stop them — v26.7.0's C ABI has 50 functions and none shuts the engine down — and under -race Go's exit path tears the sanitizer runtime down while those threads can still wake into instrumented code. One faults, Go re-raises because it is not a Go thread, and the binary dies after passing. Waiting before exit does not fix it, which was worth measuring rather than assuming: crashes at waits of none, 500ms, 2s and 5s came out 5, 3, 7 and 3 out of 48, no trend. BackgroundSchedulePool re-arms its tasks on a timer, so the threads never go quiet — a longer wait just picks a different moment to gamble on. So the step now fails on what the race detector is for. A reported data race, a failing test, a panic: red. A segfault at exit with nothing else wrong: green, with a warning. Coverage is unchanged — this is only how the step reads its own result — and it is not a blanket exemption: a run that segfaults *and* has a real failure still fails, which is one of the cases exercised below. Verified in a Linux container and on macOS, against a build that reproduces the crash: an ordinary pass exits 0; a run that hits the exit segfault exits 0 and says so; a run that hits it while also failing a test exits 1; an injected data race exits 1; an injected failing test and an injected panic exit 1. Remove this once the engine can be shut down. chdb-core has the machinery already — GlobalThreadPool::shutdown() in src/Common/ThreadPool.h — and no C entry point calls or exposes it. Then the plain exit status is trustworthy again and this wrapper should go. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"The log mentions a segfault" covered more than intended. A fault during a test — #46, which was a real one — also says signal: segmentation fault somewhere, and would have been waved through with no failing test or panic to catch it. The two are distinguishable, and by more than wording. A fault on a thread Go owns gets the runtime's own handler, which prints SIGSEGV: segmentation violation and a goroutine dump before dying. The exit-time one lands on a libchdb thread, so Go's badsignal re-raises and there is no dump at all — only the harness line. Checked against both: #46's CI log has the runtime header and no harness line; the exit-time crash has the harness line and no header. So a runtime signal header now fails the step outright, and the exemption wants positive evidence — a PASS line with the segfault on the very next one, which is what "every test in this package passed, then the binary died" looks like. Re-ran the injected data race, failing test and panic against the tightened version, and replayed both real crash logs through the new rules offline: the in-test crash is rejected, the exit-time crash is exempted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@wudidapaopao please review this PR |
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.
main has been red since #45 on one step, and not for anything in this repository.
go test -racereports every test as PASS and then the process segfaults on the wayout, so the step fails on an exit status that says nothing about the tests.
What the core dump says
runtime.badsignalis Go's path for a signal on a thread it does not own — a Cthread.
__tsan_finiand__run_exit_handlersare on the stack, and 33 threadsare parked in
__pthread_cond_wait_commoninside libchdb.So the engine's process-global ClickHouse pools are still alive at exit, because
nothing can stop them: v26.7.0's C ABI has 50 functions and none shuts the engine
down. Under
-race, Go's exit path runsracefini→__tsan_fini, tearing thesanitizer runtime down while those threads can still wake into instrumented code.
One faults, Go re-raises because it is not a Go thread, and the binary dies after
every test has passed.
This is why it only happens under
-race: without it, the same leaked threads arekilled by
exit_groupand nobody notices.Waiting does not fix it
Worth measuring rather than assuming. Crashes over 48 runs, six in parallel:
No trend.
BackgroundSchedulePoolre-arms its tasks on a timer, so the threadsnever go quiet — a longer wait just picks a different moment to gamble on. (Those
counts are non-zero exits, which at six-way parallelism include some genuine
contention failures, so they overstate the segfault rate. The absence of a trend is
the point.)
What changes
The step fails on what the race detector is for: a reported data race, a failing
test, a panic. A segfault at exit with nothing else wrong passes, with a warning.
Coverage is unchanged — this is only how the step reads its own result — and it is
not a blanket exemption. A run that segfaults and fails a test still fails.
Verified
In a Linux container and on macOS, against a build that reproduces the crash:
When to remove this
Once the engine can be shut down. chdb-core has the machinery already —
GlobalThreadPool::shutdown()insrc/Common/ThreadPool.h— and no C entry pointcalls or exposes it. Then the plain exit status is trustworthy and this wrapper
should go.
🤖 Generated with Claude Code
Note
Judge the race detector test step by inspecting output rather than exit status
go test -racethat parses output to determine pass/fail instead of relying on the raw exit code.PASSimmediately followed bysignal: segmentation fault.::warning::instead of a::error::.Macroscope summarized 12cb828.