Skip to content

fix: prevent crashes when output is piped#1048

Merged
aeppling merged 10 commits into
rtk-ai:developfrom
ashwingopalsamy:master
May 29, 2026
Merged

fix: prevent crashes when output is piped#1048
aeppling merged 10 commits into
rtk-ai:developfrom
ashwingopalsamy:master

Conversation

@ashwingopalsamy
Copy link
Copy Markdown
Contributor

Summary

  • Fix SIGABRT crash when rtk output is piped (e.g. rtk git log | head) by resetting SIGPIPE to the default handler at process startup (panic/SIGABRT on Broken pipe while writing stdout #1004)
  • Rust ignores SIGPIPE by default; combined with panic = "abort" in the release profile, any write to a closed pipe triggers SIGABRT + coredump instead of silent exit
  • Follows the conventions already used in this repo behind cfg(unix) focusing on Unix based systems, no impact on Windows platform.

Test plan

  • cargo fmt --all && cargo clippy --all-targets && cargo test
  • Manual testing: rtk git log --oneline -50 | head -1 exits cleanly (no SIGABRT, no exit 134)
  • Manual testing: rtk ls | head -1 exits cleanly
  • Integration test: cargo test --ignored test_broken_pipe_does_not_crash

Important: All PRs must target the develop branch (not master).
See CONTRIBUTING.md for details.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 6, 2026

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ aeppling
❌ ashwingopalsamy
You have signed the CLA already but the status is still pending? Let us recheck it.

@pszymkowiak pszymkowiak added bug Something isn't working effort-small Quelques heures, 1 fichier labels Apr 6, 2026
@pszymkowiak
Copy link
Copy Markdown
Collaborator

[w] wshm · Automated triage by AI

📊 Automated PR Analysis

🐛 Type bug-fix
🟡 Risk medium

Summary

Fixes a SIGABRT crash when rtk output is piped to commands like head by resetting the SIGPIPE signal handler to its default at process startup. Rust ignores SIGPIPE by default, and combined with panic="abort" in the release profile, writes to a closed pipe caused a coredump instead of a silent exit. Adds a Unix-only libc dependency and an integration test.

Review Checklist

  • Tests present
  • Breaking change
  • Docs updated

Linked issues: #1004


Analyzed automatically by wshm · This is an automated analysis, not a human review.

@pszymkowiak
Copy link
Copy Markdown
Collaborator

Hey @ashwingopalsamy, the fix itself is correct — signal(SIGPIPE, SIG_DFL) is the right approach for #1004. Tested locally, the build succeeds on macOS. A few things to address before merge:

P0 — Windows build will fail

// #[cfg(unix)]     ← currently commented out
unsafe {
    libc::signal(libc::SIGPIPE, libc::SIG_DFL);
}

libc is declared as [target.'cfg(unix)'.dependencies] in Cargo.toml, so it's not available on Windows. Without the #[cfg(unix)] guard, the Windows build will fail with "cannot find crate libc" and SIGPIPE doesn't exist on Windows anyway.

Fix: uncomment the #[cfg(unix)] line so the block is gated:

#[cfg(unix)]
unsafe {
    libc::signal(libc::SIGPIPE, libc::SIG_DFL);
}

P1 — Noise causing the CONFLICTING state

The PR includes unrelated changes:

  • Cargo.toml / Cargo.lock: version bump 0.34.1 → 0.34.3
  • release-please-manifest.json: same bump
  • CHANGELOG.md: 32 lines from release 0.34.3 that already exist on develop

These happen when you branch from an older commit. release-please handles version bumps and changelog automatically on release, so please drop these files.

Fix:

git fetch origin
git rebase origin/develop
# Keep develop's versions for Cargo.toml/Cargo.lock/CHANGELOG.md/release-please-manifest.json
git push --force-with-lease

P2 — Typo in the test

Line 2485: git log --online--oneline. Test still passes (only checks exit != 134), but worth fixing.

Thanks — this is a real bug, the fix just needs a bit of cleanup!

@ashwingopalsamy
Copy link
Copy Markdown
Contributor Author

@pszymkowiak - I've attempted to fix the suggestions. Please review.

@pszymkowiak
Copy link
Copy Markdown
Collaborator

Thanks @ashwingopalsamy, and sorry for the long delay. Re-reviewed — you've addressed all three points:

  • P0: the #[cfg(unix)] guard is in place ✅
  • P1: the release-please noise (Cargo.toml/lock, CHANGELOG, manifest) is gone — the diff is just main.rs now ✅
  • P2: --oneline typo fixed ✅

One last thing: the PR still shows CONFLICTINGmain.rs moved on develop since April, so the branch needs a rebase:

git fetch origin && git rebase origin/develop && git push --force-with-lease

After that + green CI it's good to merge — the fix itself is correct and #1004 is still unfixed on develop.

@aeppling
Copy link
Copy Markdown
Contributor

Hey, conflicts solved

@aeppling
Copy link
Copy Markdown
Contributor

LGTM

Thanks for contributing to RTK !

@aeppling aeppling merged commit acfd6c5 into rtk-ai:develop May 29, 2026
10 of 11 checks passed
@aeppling aeppling mentioned this pull request May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working effort-small Quelques heures, 1 fichier

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants