Skip to content

test(crasht): fix CentOS SIGPIPE tests #1233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Sep 19, 2025
Merged

test(crasht): fix CentOS SIGPIPE tests #1233

merged 3 commits into from
Sep 19, 2025

Conversation

gyuheon0h
Copy link
Contributor

@gyuheon0h gyuheon0h commented Sep 19, 2025 •
edited
Loading

What does this PR do?

  1. We have SIGPIPE tests in place to make sure that the crashtracker is not bothered by SIGPIPE.
  2. One of the things that the tests do is this; Verifies that SIGPIPE is even emitted (this is done by opening a pipe, closing one end, and writing to the other)
  3. There has been an upstream change in Rust that suppresses SIGPIPE being emitted by writing to socket with MSG_NOSIGNAL flag.
  4. This has caused the check in (2) to fail, since we are no longer emitting a SIGPIPE
  5. Haven't looked into why only CentOS 7 is affected

Changing the write to the pipe from writing to Rust UnixStream to using raw system call write() to the write fd, without wrapping it in UnixStream fixes the test.

  1. We fail with SIGPIPE because we are writing to a socket that has been dropped from the other side
  2. We verify that we emit a SIGPIPE
  3. We verify all the downstream checks, such as checking that the SIGPIPE doesnt affect the crashtracker.

Motivation

We received reports of bin tests, sigpipe and sigpipe_sigstack failing both on main and feature branches.

Additional Notes

Anything else we should know when reviewing?

How to test the change?

Describe here in detail how the change can be validated.

@gyuheon0h gyuheon0h marked this pull request as ready for review September 19, 2025 16:39
@gyuheon0h gyuheon0h requested a review from a team as a code owner September 19, 2025 16:39
@morrisonlevi morrisonlevi changed the title [crashtracker] CentOS SIGPIPE tests fix fix(crasht): CentOS SIGPIPE tests Sep 19, 2025
@morrisonlevi morrisonlevi changed the title fix(crasht): CentOS SIGPIPE tests test(crasht): fix CentOS SIGPIPE tests Sep 19, 2025
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.65%. Comparing base (31cf340) to head (cf79614).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1233   +/-   ##
=======================================
  Coverage   71.65%   71.65%           
=======================================
  Files         354      354           
  Lines       56053    56063   +10     
=======================================
+ Hits        40162    40172   +10     
  Misses      15891    15891
Components Coverage Δ
datadog-crashtracker 49.30% <ø> (+0.06%) ⬆️
datadog-crashtracker-ffi 5.93% <ø> (ø)
datadog-alloc 98.73% <ø> (ø)
data-pipeline 90.30% <ø> (ø)
data-pipeline-ffi 88.19% <ø> (ø)
ddcommon 84.29% <ø> (ø)
ddcommon-ffi 73.84% <ø> (ø)
ddtelemetry 59.98% <ø> (ø)
ddtelemetry-ffi 21.24% <ø> (ø)
dogstatsd-client 83.26% <ø> (ø)
datadog-ipc 82.39% <ø> (ø)
datadog-profiling 76.90% <ø> (ø)
datadog-profiling-ffi 62.12% <ø> (ø)
datadog-sidecar 37.08% <ø> (ø)
datdog-sidecar-ffi 11.37% <ø> (ø)
spawn-worker 55.35% <ø> (ø)
tinybytes 92.22% <ø> (ø)
datadog-trace-normalization 98.24% <ø> (ø)
datadog-trace-obfuscation 94.17% <ø> (ø)
datadog-trace-protobuf 77.10% <ø> (ø)
datadog-trace-utils 89.75% <ø> (ø)
datadog-tracer-flare 54.52% <ø> (ø)
datadog-log 76.31% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gyuheon0h
Copy link
Contributor Author

/merge

@dd-devflow-routing-codex
Copy link

dd-devflow-routing-codex bot commented Sep 19, 2025 •
edited
Loading

View all feedbacks in Devflow UI.

2025-09-19 17:21:49 UTC ℹ️ Start processing command /merge

2025-09-19 17:21:54 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 46m (p90).

2025-09-19 17:46:21 UTC ❌ MergeQueue: The checks failed on this merge request

Tests failed on this commit e16f165:

What to do next?

  • Investigate the failures and when ready, re-add your pull request to the queue!
  • If your PR checks are green, try to rebase/merge. It might be because the CI run is a bit old.
  • Any question, go check the FAQ.

danielsn
danielsn approved these changes Sep 19, 2025
View reviewed changes
bin_tests/src/modes/unix/test_001_sigpipe.rs
Comment on lines +91 to +98

// Use raw write() syscall instead of Rust's write_all() to avoid MSG_NOSIGNAL
let writer_raw_fd = writer_fd.as_raw_fd();
let write_result =
unsafe { libc::write(writer_raw_fd, b"Hello".as_ptr() as *const libc::c_void, 5) };

if write_result != -1 {
anyhow::bail!("Expected write to fail with SIGPIPE, but it succeeded");
Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed offline moving this into its own function, and adding a test that it does indeed raise a SIGPIPE. Followup work since this PR is needed to unblock CI.

@gyuheon0h
Copy link
Contributor Author

/merge

@dd-devflow-routing-codex
Copy link

dd-devflow-routing-codex bot commented Sep 19, 2025 •
edited
Loading

View all feedbacks in Devflow UI.

2025-09-19 17:56:01 UTC ℹ️ Start processing command /merge

2025-09-19 17:56:06 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 46m (p90).

2025-09-19 18:18:41 UTC ℹ️ MergeQueue: This merge request was merged

@dd-mergequeue dd-mergequeue bot merged commit f198cdb into main Sep 19, 2025
37 checks passed
@dd-mergequeue dd-mergequeue bot deleted the gyuheon0h/centos7-test branch September 19, 2025 18:18
@gyuheon0h gyuheon0h mentioned this pull request Sep 19, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ekump ekump ekump approved these changes

@danielsn danielsn danielsn approved these changes

Assignees

No one assigned

Labels

mergequeue-status: done

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gyuheon0h @codecov-commenter @ekump @danielsn @morrisonlevi