Skip to content

fix: abort pipeline on deadlock detection when recovery is disabled - #252

Merged
nh13 merged 1 commit into
mainfrom
nh/abort-on-deadlock
Apr 10, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/abort-on-deadlock

Conversation

@nh13

@nh13 nh13 commented Apr 10, 2026

Copy link
Copy Markdown
Member

Summary

  • When deadlock detection fires and --deadlock-recover is not enabled, the pipeline now aborts with a clear error instead of logging a warning and hanging indefinitely
  • Workers observe has_error() on their next iteration and exit gracefully — no panics, proper output cleanup
  • Error uses TimedOut kind with an actionable message suggesting --deadlock-recover
  • Both monitor paths fixed: run_monitor_loop (FASTQ pipeline) and the inline BAM monitor thread

Before

Deadlock detected → warning logged → progress timer reset → pipeline continues hanging → repeated warnings every 10s → user must Ctrl-C

After

Deadlock detected → warning logged with diagnostics → set_error() called → all workers exit on next has_error() check → pipeline exits with non-zero status and clear message

Test plan

  • All 2347 existing tests pass
  • cargo ci-fmt clean
  • cargo ci-lint clean
  • Manual verification: run with --deadlock-timeout 5 (no --deadlock-recover) on a workload that deadlocks — should exit with error after 5s instead of hanging

When deadlock detection fires and --deadlock-recover is not enabled,
the pipeline now sets an error and exits instead of logging a warning
and continuing to hang indefinitely. Workers observe has_error() on
their next iteration and exit gracefully.

Both monitor paths are fixed:
- run_monitor_loop (base.rs) — used by FASTQ pipeline
- inline monitor thread (bam.rs) — used by BAM pipeline
@nh13
nh13 temporarily deployed to github-actions April 10, 2026 06:53 — with GitHub Actions Inactive
@nh13
nh13 marked this pull request as ready for review April 10, 2026 06:53
@nh13 nh13 added the enhancement New feature or request label Apr 10, 2026
@codecov

codecov Bot commented Apr 10, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.27%. Comparing base (91c0231) to head (706c295).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/lib/unified_pipeline/bam.rs 0.00% 8 Missing ⚠️
src/lib/unified_pipeline/base.rs 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #252      +/-   ##
==========================================
- Coverage   89.28%   89.27%   -0.02%     
==========================================
  Files         119      119              
  Lines       57770    57784      +14     
==========================================
+ Hits        51582    51584       +2     
- Misses       6188     6200      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai

coderabbitai Bot commented Apr 10, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7e3d3dc7-3f15-4aac-a631-05e5b055923e

📥 Commits

Reviewing files that changed from the base of the PR and between 91c0231 and 706c295.

📒 Files selected for processing (2)
  • src/lib/unified_pipeline/bam.rs
  • src/lib/unified_pipeline/base.rs

📝 Walkthrough

Walkthrough

The pull request modifies deadlock detection handling in the unified pipeline's monitor loop across two files. Previously, check_deadlock_and_restore() was invoked without inspecting its return value. The changes now import DeadlockAction, match on the function's return value, and when deadlock is detected, explicitly record a TimedOut error and terminate the monitor loop. This ensures the pipeline stops with an appropriate error when deadlock recovery is disabled.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately captures the main change: aborting the pipeline on deadlock detection when recovery is disabled.
Description check ✅ Passed The description thoroughly explains the problem, solution, behavior changes, and test plan — all directly related to the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nh/abort-on-deadlock

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nh13
nh13 merged commit 906af30 into main Apr 10, 2026
7 of 9 checks passed
@nh13
nh13 deleted the nh/abort-on-deadlock branch April 10, 2026 07:05

This branch was previously deployed

1 inactive deployment
github-actions — 706c2952 Deployed Apr 10, 2026 by nh13 via coverage #1013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant