ci: if test core dumps, upload dump as build artifact #2906
Conversation
📝 WalkthroughWalkthroughUpdates CI workflow: adjusts Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2024-11-08T09:27:23.601ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/tests-rs-package.yml (2)
201-206: Consider more restrictive permissions for /cores directory.While 777 permissions work in this CI container environment, it's overly permissive. Consider using 755 or 775 instead to follow the principle of least privilege.
Apply this diff to use more restrictive permissions:
- name: Configure core dumps run: | sudo mkdir /cores - sudo chmod 777 /cores + sudo chmod 755 /cores # Core filenames will be of the form executable.pid.timestamp: sudo bash -c 'echo "/cores/%e.%p.%t" > /proc/sys/kernel/core_pattern'
246-251: dSYM files are macOS-specific but workflow runs on Linux.The script collects
.dSYMfiles, which are macOS debug symbols, but this workflow runs onubuntu-24.04. While harmless, this check is unnecessary on Linux. The.dwpfiles are the appropriate debug symbol format for Linux.Apply this diff to remove unnecessary macOS-specific debug symbol handling:
- for extra in target/debug/deps/${BIN_PREFIX}-*.dSYM target/debug/${BIN_PREFIX}.dSYM \ - target/debug/deps/${BIN_PREFIX}-*.dwp target/debug/${BIN_PREFIX}.dwp; do + for extra in target/debug/deps/${BIN_PREFIX}-*.dwp target/debug/${BIN_PREFIX}.dwp; do if [[ -e "$extra" ]]; then cp -a "$extra" "${ARTIFACT_DIR}/binaries/" fi done
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests-rs-package.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: lklimek
Repo: dashpay/platform PR: 2318
File: .github/workflows/tests-build-image.yml:45-45
Timestamp: 2024-11-13T10:31:30.891Z
Learning: In the dashpay/platform repository, changes to `.github/workflows/tests-build-image.yml` that switch the Docker image platform from `linux/arm64` to `linux/amd64` for testing purposes are acceptable when required to run on GitHub-hosted runners. ARM64 testing is covered on the testnet.
📚 Learning: 2024-11-08T09:27:23.601Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2316
File: .github/actions/librocksdb/action.yaml:22-28
Timestamp: 2024-11-08T09:27:23.601Z
Learning: Prefer to use `librocksdb/${{ inputs.version }}/${{ runner.os }}/${{ runner.arch }}` as the cache key for caching librocksdb in workflows; it's sufficient.
Applied to files:
.github/workflows/tests-rs-package.yml
🔇 Additional comments (2)
.github/workflows/tests-rs-package.yml (2)
209-211: LGTM!The ulimit configuration correctly enables unlimited core dump generation, which is essential for capturing crash information when tests fail.
255-264: LGTM!The artifact upload configuration is well-designed with appropriate retention period, graceful handling of missing files, and helpful usage documentation.
|
Don't we need something that cleans these up from time to time? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/tests-rs-package.yml:
- Around line 201-206: The workflow step that configures core dump pattern
currently creates /cores and writes to /proc/sys/kernel/core_pattern but misses
enabling core dumps via ulimit; add a command to set the core file size limit to
unlimited (e.g., run ulimit -c unlimited) in the same shell where tests execute
(either add it to the "Configure core dumps" step before writing the pattern and
ensure subsequent commands run in the same shell, or place ulimit -c unlimited
at the start of the "Run tests" step) so the kernel will actually produce core
files when a crash occurs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests-rs-package.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: lklimek
Repo: dashpay/platform PR: 2318
File: .github/workflows/tests-build-image.yml:45-45
Timestamp: 2024-11-13T10:31:30.891Z
Learning: In the dashpay/platform repository, changes to `.github/workflows/tests-build-image.yml` that switch the Docker image platform from `linux/arm64` to `linux/amd64` for testing purposes are acceptable when required to run on GitHub-hosted runners. ARM64 testing is covered on the testnet.
📚 Learning: 2024-11-08T09:27:23.601Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2316
File: .github/actions/librocksdb/action.yaml:22-28
Timestamp: 2024-11-08T09:27:23.601Z
Learning: Prefer to use `librocksdb/${{ inputs.version }}/${{ runner.os }}/${{ runner.arch }}` as the cache key for caching librocksdb in workflows; it's sufficient.
Applied to files:
.github/workflows/tests-rs-package.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (2)
.github/workflows/tests-rs-package.yml (2)
217-251: Well-structured crash artifact collection.The script handles edge cases properly:
- Uses
set -euo pipefailfor strict error handling- Uses
compgen -Gto safely check for glob matches- Correctly transforms package names (
-to_) to match Rust's binary naming convention- Includes both dSYM (macOS) and dwp (DWARF) debug symbols
One observation: dSYM files are macOS-specific and won't exist on Ubuntu runners, but including them doesn't hurt and makes the workflow portable if runners change in the future.
253-262: LGTM! Retention policy addresses cleanup concern.The
retention-days: 3setting addresses QuantumExplorer's question about artifact cleanup—GitHub will automatically delete these artifacts after 3 days. Theif-no-files-found: ignorecorrectly handles the case where no core dumps were produced.
There was a problem hiding this comment.
Pull request overview
This PR enhances CI test failure diagnostics by adding core dump collection capabilities to the Rust package test workflow. When tests fail and produce core dumps, the workflow now captures both the core dumps and associated binaries, archives them, and uploads them as GitHub artifacts for debugging purposes.
Changes:
- Configure core dump generation with unlimited size and custom file naming pattern
- Collect core dumps and test binaries on test failure
- Upload crash artifacts as short-lived (3 days) GitHub artifacts
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Issue being fixed or feature implemented
When tests are killed, we want to be able to retrieve core dump and affected binary to investigate
What was done?
Extended test workflow to upload core dump and binary to github artifacts.
How Has This Been Tested?
https://github.com/dashpay/platform/actions/runs/20302691431
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.