Skip to content

test: surface compute sanitizer logs - #4752

Open
WilliamK112 wants to merge 5 commits into
NVIDIA:release/26.08from
WilliamK112:codex/surface-sanitizer-logs
Open

test: surface compute sanitizer logs#4752
WilliamK112 wants to merge 5 commits into
NVIDIA:release/26.08from
WilliamK112:codex/surface-sanitizer-logs

Conversation

@WilliamK112

@WilliamK112 WilliamK112 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • print fresh Compute Sanitizer log excerpts to stderr when the sanitizer wrapper exits nonzero
  • limit emitted sanitizer logs to the last 80 lines so CI output stays bounded
  • isolate each wrapper invocation's sanitizer log basename so parallel surefire forks do not print sibling fork logs
  • forward TERM/INT/HUP from the wrapper to the running compute-sanitizer process and wait for it before exiting
  • preserve the sanitizer exit status, and only override wait with a forwarded signal status when wait was signal-interrupted

Closes #1337.

Validation

  • bash -n build/sanitizer-java/bin/java
  • fake failing compute-sanitizer wrapper test: verified stderr reports the sanitizer exit status, prints only the current fork's sanitizer_for_pid_*.log, and limits a 120-line fake log to lines 041-120
  • fake successful compute-sanitizer wrapper test: verified stderr stays empty and the wrapper exits 0
  • fake long-running compute-sanitizer wrapper test: sent TERM to the wrapper, verified the child compute-sanitizer received TERM and the wrapper exited 143 after waiting for the child
  • git diff --check origin/main...HEAD
  • git diff --check
  • uvx --from shellcheck-py shellcheck --format=gcc ... build/sanitizer-java/bin/java against LF-normalized/committed content
  • uvx pre-commit run --files build/sanitizer-java/bin/java --verbose
  • uvx pre-commit run --all-files --verbose
  • GitHub checks after latest push: license-header-check, shell-check, signoff-check, Greptile Review, and pre-commit.ci - pr all passed

Revalidated on 2026-07-20 before retargeting to release/26.08:

  • git merge-tree $(git merge-base upstream/release/26.08 HEAD) upstream/release/26.08 HEAD showed no conflict markers or changed-in-both paths for this wrapper change
  • bash -n build/sanitizer-java/bin/java
  • git diff --check upstream/release/26.08...HEAD
  • git diff --check
  • local fake compute-sanitizer wrapper harness passed failure-log tailing/scoping, success quietness, and TERM forwarding cases
  • bash -n plus uvx --from shellcheck-py shellcheck --shell=bash --format=gcc on the committed LF blob
  • SKIP=check-clang-format-version uvx pre-commit run --files build/sanitizer-java/bin/java --verbose passed/skipped only non-applicable hooks; the unskipped always-run clang-format-version hook requires thirdparty/cudf, which is not initialized in this local checkout

@WilliamK112
WilliamK112 marked this pull request as ready for review July 2, 2026 03:33
@WilliamK112
WilliamK112 requested a review from a team as a code owner July 2, 2026 03:33
@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR replaces the simple exec compute-sanitizer … with a background-job wrapper that forwards TERM/INT/HUP, uses a BASHPID+RANDOM log token to scope per-fork log filenames, and tails the last 80 lines of those logs to stderr when the sanitizer exits non-zero.

  • Signal forwarding: compute-sanitizer is launched as a background job; trap handlers call forward_signal, which records received_signal and forwards the signal to the child PID, followed by wait and a self-kill to propagate the signal exit status to surefire.
  • Log isolation & tailing: the --log-file argument embeds a unique ${BASHPID:-$$}_${RANDOM}_${RANDOM} token so parallel surefire forks cannot pick up each other's log files; on failure, find … -name "${log_basename}_*.log" and tail -n 80 emit the last 80 lines per log.
  • False-positive diagnostic on signal-interrupted wait: when surefire cancels a fork with TERM, wait "$sanitizer_pid" returns 128+15=143 and received_signal is already set, but the error-output block at line 51 fires unconditionally — printing "compute-sanitizer failed with exit code 143" even though no sanitizer error occurred.

Confidence Score: 4/5

Mostly safe to merge; the core signal-forwarding and log-isolation mechanics work correctly, but a one-line guard is missing that causes every signal-interrupted wait to produce a misleading sanitizer-failure message in CI output.

The wrapper correctly forwards signals, scopes log filenames per-fork, and tails logs on genuine failures. The one defect is that when wait returns early because the shell received a signal (status = 128+signum), the error-output block fires and prints "compute-sanitizer failed with exit code 143" — a false positive that would appear on every surefire timeout or build cancellation.

Files Needing Attention: build/sanitizer-java/bin/java — the signal-interrupted-wait false-positive at line 51

Important Files Changed

Filename Overview
build/sanitizer-java/bin/java Adds signal forwarding (TERM/INT/HUP), per-fork log isolation via BASHPID+RANDOM token, and log tailing on non-zero exit. Signal-interrupted wait returns 128+signum, causing the error-output block to misfire and print a false sanitizer-failure diagnostic; the wait after kill $$ is also unreachable dead code.

Reviews (6): Last reviewed commit: "test: preserve sanitizer wrapper signals" | Re-trigger Greptile

Comment thread build/sanitizer-java/bin/java Outdated
Comment thread build/sanitizer-java/bin/java Outdated
Comment thread build/sanitizer-java/bin/java
@WilliamK112

Copy link
Copy Markdown
Contributor Author

I reproduced the current pre-commit.ci failure locally by merging this branch onto current origin/main: the clang-format hook modified native source files introduced by the latest main after the cudf submodule sync (#4777), not this sanitizer wrapper change. I opened #4778 with only the hook-generated formatting changes, and #4778 is now passing pre-commit.ci, Greptile, and signoff. Once that lands or the same formatting is applied on main, this PR's remaining red pre-commit should be resolved by rerun/rebase.

@ttnghia

ttnghia commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Do not dump the entire log - it may be huge. Just take the last few lines that are enough for quick inspection.

ttnghia added a commit that referenced this pull request Jul 2, 2026
## Summary
- Run the repository clang-format hook after the latest cudf submodule
sync on `main`
- Apply only the mechanical native-source formatting changes produced by
the existing pre-commit configuration
- Update the expired copyright years reported by `license-header-check`

## Why
A local merge of #4752 onto current `origin/main` reproduced the current
`pre-commit.ci - pr` failure: the `clang-format` hook modified native
source files after the cudf sync in #4777. This PR applies those
hook-generated changes directly on top of current `main`, then fixes the
license-header files reported by the maintainer-triggered check.

## Validation
- `pre-commit run clang-format --all-files --verbose` initially
reproduced the same files-were-modified failure
- GitHub `pre-commit.ci - pr` passes on head `ef703bfb`
- Local equivalent `license-header-check` regex passes for the 23
included changed files
- `uvx pre-commit run --files src/main/cpp/faultinj/faultinj.cu
src/main/cpp/profiler/ProfilerJni.cpp
src/main/cpp/profiler/init_nvtxw.cpp
src/main/cpp/profiler/spark_rapids_profile_converter.cpp
src/main/cpp/src/KudoGpuSerializerJni.cpp src/main/cpp/src/zorder.cu
src/main/cpp/tests/exception_with_row_index.cpp
src/main/cpp/tests/multiply.cpp --verbose` passes
- `git diff --check` passes
- `git diff --check origin/main...HEAD` passes

---------

Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
Co-authored-by: Nghia Truong <7416935+ttnghia@users.noreply.github.com>
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
@WilliamK112
WilliamK112 force-pushed the codex/surface-sanitizer-logs branch from 84895da to 40d6012 Compare July 4, 2026 04:20
@WilliamK112

Copy link
Copy Markdown
Contributor Author

Updated this PR for the latest review feedback:

  • rebased onto current main after Format native sources after cudf sync #4778 landed, so the previous unrelated clang-format pre-commit failure should be gone on rerun
  • changed the sanitizer failure path to print only the last 80 lines of each matching log instead of dumping the entire file
  • fixed the signal-forwarding race Greptile flagged by only applying forwarded_status when wait returned a signal-interrupted status (>128)

Local validation passed:

  • bash -n build/sanitizer-java/bin/java
  • fake failing/successful/signal-forwarding compute-sanitizer wrapper tests
  • git diff --check origin/main...HEAD
  • uvx pre-commit run --files build/sanitizer-java/bin/java --verbose
  • uvx pre-commit run --all-files --verbose

Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
@WilliamK112
WilliamK112 force-pushed the codex/surface-sanitizer-logs branch from 40d6012 to 47ac57b Compare July 4, 2026 04:29
@nvauto

nvauto commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

NOTE: release/26.08 has been created from main. Please retarget your PR to release/26.08 if it should be included in the release.

@WilliamK112
WilliamK112 changed the base branch from main to release/26.08 July 20, 2026 15:01
@WilliamK112

WilliamK112 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Retargeted this PR to release/26.08 after the release branch notification. I checked the branch against upstream/release/26.08 with git merge-tree and revalidated the wrapper locally on 2026-07-20: syntax, diff whitespace, fake failure/success/signal-forwarding compute-sanitizer tests, shellcheck on the committed LF blob, and applicable pre-commit hooks for the changed file.

@abellina

abellina commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

build

@abellina abellina left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one comment and I was wondering if we can force a failure to see the output to add this to the PR. For example, find some kernel we have and then clearly write or read to memory outside of the range. compute-sanitizer should catch and report, that way we can see what it looks like.

Comment thread build/sanitizer-java/bin/java Outdated
Comment thread build/sanitizer-java/bin/java Outdated
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
@WilliamK112

Copy link
Copy Markdown
Contributor Author

Thanks for the review. The two code changes are now in signed-off commit 11380031:

  • the log destination and lookup both use the absolute startup WORKDIR;
  • TERM/INT/HUP are forwarded by name and then re-raised against the wrapper instead of being mapped to hardcoded exit codes.

Local fake-wrapper coverage verifies the success path, the failure path with only the final 80 log lines, the absolute log path, and TERM/HUP forwarding plus re-raise semantics. The repository license, signoff, ShellCheck, and Greptile checks are green on this head (pre-commit.ci is still pending at the time of this comment).

For the requested real compute-sanitizer failure output: I checked the available automation before claiming proof. The public GitHub Actions jobs do not have a GPU, and the Blossom GPU premerge path runs ci/premerge-build.sh, which does not enable -DUSE_SANITIZER=ON. My macOS host therefore cannot produce an authentic CUDA out-of-bounds report.

I can prepare a short-lived proof-only commit that enables USE_SANITIZER for one deliberately failing test/kernel, have a maintainer trigger the Blossom build, capture the resulting wrapper output for this PR, and then revert the proof-only commit before merge. Would you prefer that route, or is there a maintainer-side way to invoke the GPU job with -DUSE_SANITIZER=ON without changing the branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make the error log more friendly when Compute Sanitizer fails the build.

4 participants