Skip to content

fix(pipeline): admit the gap-filler serial to Decode under memory-high - #787

Merged
nh13 merged 1 commit into
mainfrom
746-deadlock/nhomer/decode-next-seq-bypass
Aug 18, 2026
Merged

nh13 merged 1 commit into
mainfrom
746-deadlock/nhomer/decode-next-seq-bypass

Conversation

@nh13

@nh13 nh13 commented Aug 16, 2026 •

Copy link
Copy Markdown
Member

Draft, stacked on #764 (base = 746/nhomer/bound-consensus-memory-under-writer-stall). It fixes a deadlock that #764's tests still hit — see below. Rebase onto #764 when that lands.

The problem: #764's deadlock is not fixed, and green CI is masking it

#764's three test_pipeline_memory_backpressure tests pass in CI but reliably hang on a loaded host. The hang is a real, scheduling-sensitive deadlock in the Decode step, not flaky tests:

  • The Decode step gates new work with a blanket q3_reorder_state.is_memory_high() check before popping Q2b (bam.rs, try_step_decode). It was copied from the Decompress step, whose comment argues the guard is deadlock-safe because "Q1 is FIFO, so next_seq for the reorder buffer has already been produced."
  • That invariant does not hold for Decode. When a slow writer backs the pipeline up and the Q3 reorder buffer reaches its high-water mark, the guard also stops Decode from decoding the very serial the reorder buffer is waiting on. That serial never reaches Q3, Group starves on it, and the pipeline wedges with the reader gated — exactly the OOM-avoidance path fix(pipeline): enforce the queue memory budget at the Read step #764 adds.

Evidence (controlled A/B on the loaded host, same commit, only this guard toggled):

Decode Q3 is_memory_high guard 4 backpressure tests
ON (as in #764) 4/4 TIMEOUT (hang)
OFF 4/4 pass

CI's lighter scheduling doesn't trip the race, so #764 goes green — a false negative for this deadlock class.

The fix

Replace the blanket guard with the reorder buffer's own per-serial can_proceed, applied after the pop:

  • It always admits the gap-filler (serial == next_seq), even over the memory limit, so the serial Group is waiting on can never be starved.
  • It backpressures only future serials, once the buffer is half full.
  • A future batch rejected by can_proceed is returned to Q2b so another worker can still reach the gap-filler — avoiding the "all workers hold a non-next_seq batch and nobody can produce next_seq" deadlock the code hit when can_proceed was previously wired into Decompress (see that step's comment).

On the healthy path (memory below the high-water mark) can_proceed is always true, so throughput is unchanged — the new backpressure engages only under the exact memory-high condition that used to deadlock.

Validation

  • The four backpressure tests pass on the loaded reproducer across 3 repeated runs (they reliably hang without this change).
  • Full workspace suite: 6675/6675 pass. cargo ci-fmt and cargo ci-lint clean.

Caveat worth raising separately

CI cannot currently catch this deadlock class (it needs constrained scheduling to reproduce). A follow-up that reproduces it under load in CI would keep it from regressing silently.

Risk: output for grouping, consensus, sort order, corrected UMIs, and metrics: none; unsafe: none, and no CLAUDE.md allowlist update; memory bound, queue capacity, and thread/backpressure policy: changed for Decode under memory-high conditions.

Fix: Decode applies per-serial admission after popping Q2b. It requeues future serials and admits the required Q3 gap-filler serial. This prevents stalls while preserving serial order.

Validation:

  • Four backpressure tests pass across three loaded-reproducer runs.
  • Full workspace suite passes: 6675/6675 tests.
  • Formatting and lint checks pass.

@nh13
nh13 deployed to github-actions August 16, 2026 05:57 — with GitHub Actions Active
@coderabbitai

coderabbitai Bot commented Aug 16, 2026 •

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 47188a32-371f-49ce-a87b-16de45cb6b20

📥 Commits

Reviewing files that changed from the base of the PR and between e69b17a and c95e22a.

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

Included review availability: 0 reviews are currently available. Based on recent review activity, included reviews refill at 1 per hour.


Walkthrough

Decode now checks physical Q3 capacity before popping Q2b. It then applies per-serial admission, requeues future serials, and directly decodes when requeueing fails. Integration tests add read-batch control and validate budget-scaled backpressure.

Changes

Decode admission flow

Layer / File(s) Summary
Physical capacity and serial admission
src/lib/unified_pipeline/bam.rs
Decode separates physical capacity checks from serial admission. Future serials return to Q2b under pressure. Required gap-filling serials continue, including bounded direct decoding when requeueing fails. Unit tests cover admission, requeueing, fallback, and charge refunding.
Read-ahead budget validation
tests/integration/test_pipeline_memory_backpressure.rs
The stalled-pipeline helper accepts an optional read-batch size. Tests use fine-grained reads for 1 MiB versus 3 MiB comparisons and retain default behavior in other scenarios.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to c95e2

The change allows the Decode step to admit the required gap-filler while continuing to backpressure future work under high memory. No actionable merge-blocking risk remains beyond normal checks and review.

Possibly related issues

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title uses valid Conventional Commit syntax, describes the main Decode scheduling fix, and has a lowercase imperative description without a period.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@nh13 nh13 added the bug Something isn't working label Aug 16, 2026
@nh13
nh13 marked this pull request as ready for review August 16, 2026 05:58
@nh13

nh13 commented Aug 16, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai pause

@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews paused.

@codecov

codecov Bot commented Aug 16, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.36364% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.36%. Comparing base (f0d6c33) to head (c95e22a).

Files with missing lines Patch % Lines
src/lib/unified_pipeline/bam.rs 96.36% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #787   +/-   ##
=======================================
  Coverage   94.36%   94.36%           
=======================================
  Files         186      186           
  Lines      113659   113713   +54     
=======================================
+ Hits       107251   107304   +53     
- Misses       6408     6409    +1     

☔ View full report in Codecov by Harness.
📢 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.

@nh13
nh13 force-pushed the 746/nhomer/bound-consensus-memory-under-writer-stall branch 3 times, most recently from b18eb89 to 87c6537 Compare August 16, 2026 19:05
@nh13
nh13 force-pushed the 746-deadlock/nhomer/decode-next-seq-bypass branch from f840cd7 to e69b17a Compare August 16, 2026 19:46
@nh13
nh13 deployed to github-actions August 16, 2026 19:46 — with GitHub Actions Active
@nh13

nh13 commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 17, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@nh13
nh13 force-pushed the 746/nhomer/bound-consensus-memory-under-writer-stall branch from 87c6537 to d0836df Compare August 17, 2026 22:53
@nh13
nh13 force-pushed the 746-deadlock/nhomer/decode-next-seq-bypass branch from e69b17a to 20658c6 Compare August 17, 2026 23:40
@nh13
nh13 force-pushed the 746-deadlock/nhomer/decode-next-seq-bypass branch from 20658c6 to 9bde1d5 Compare August 18, 2026 01:20
@nh13
nh13 deployed to github-actions August 18, 2026 01:20 — with GitHub Actions Active
Base automatically changed from 746/nhomer/bound-consensus-memory-under-writer-stall to main August 18, 2026 02:29
#746)

The Decode step gated new work with a blanket `q3_reorder_state.is_memory_high()` check before popping Q2b. Unlike the Decompress step it was copied from — where FIFO Q1 guarantees the reorder buffer's `next_seq` was already produced and is sitting downstream or in a held slot — this is not deadlock-safe for Decode: when a slow writer backs the pipeline up and the Q3 reorder buffer reaches its high-water mark, the guard also stops Decode from decoding the very serial the reorder buffer is waiting on. That serial never reaches Q3, Group starves on it, and the whole pipeline wedges with the reader gated — the exact OOM-avoidance path this PR adds.

Replace the blanket guard with the reorder buffer's own per-serial `can_proceed`, applied after the pop: it always admits the gap-filler (`serial == next_seq`), even over the memory limit, and backpressures only future serials once the buffer is half full. A future batch rejected by `can_proceed` is returned to Q2b so another worker can still reach the gap-filler, which avoids the "all workers hold a non-next_seq batch and nobody can produce next_seq" deadlock the code hit when `can_proceed` was previously wired into Decompress. On the healthy path (memory below the high-water mark) `can_proceed` is always true, so throughput is unchanged.

The deadlock is scheduling-sensitive: CI's lighter load did not trigger it, so the three backpressure tests passed in CI while reliably hanging on a loaded host. A controlled A/B on that host (same code, only this guard toggled) confirmed the guard as the cause. With this change the four backpressure tests pass on that host across repeated runs.
@nh13
nh13 force-pushed the 746-deadlock/nhomer/decode-next-seq-bypass branch from 9bde1d5 to c95e22a Compare August 18, 2026 02:35
@nh13
nh13 deployed to github-actions August 18, 2026 02:35 — with GitHub Actions Active
@nh13

nh13 commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 18, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@nh13
nh13 merged commit c900eb0 into main Aug 18, 2026
16 checks passed
@nh13
nh13 deleted the 746-deadlock/nhomer/decode-next-seq-bypass branch August 18, 2026 05:49
@nh13 nh13 mentioned this pull request Aug 18, 2026
nh13 added a commit that referenced this pull request Aug 23, 2026
The multi-threaded pipeline serialized decode under load: at 16 threads the
decode stage ran at ~3.6x effective parallelism instead of ~15x, making
consensus/group/filter/sort 1.7x (t8) to 3.3x avg / 8.5x worst (t16) slower
than v0.6.0.

#787 (fixing the #746 deadlock) gated future-serial admission at
`heap_bytes < effective_limit / 2` with `effective_limit = min(--max-memory,
512 MiB)`. The Q3 decoded-record reorder buffer therefore stopped admitting
anything but next_seq once it held ~256 MiB -- a ceiling that scales with
neither --max-memory nor thread count; with N decode workers it is crossed
almost immediately and decode collapses to serial.

Bound Q3 admission by serial skew instead, as the reorder buffer's own
docstring prescribes: admit next_seq unconditionally (the #746 core), and a
future serial iff within [next_seq, next_seq + W) and under a raw memory
backstop, with W = clamp(4 * num_threads, .., queue_capacity). Total in-flight
memory stays bounded upstream by the Read admission gate, so decode no longer
needs a per-stage byte throttle that doubled as a parallelism cap. Scoped to
Q3 via a new `window` field (0 = legacy byte-threshold behavior).

Output is record-identical to v0.6.0; the #746 backpressure suite passes and a
tight --max-memory run stays bounded without wedging. Adds
test_reorder_buffer_state_windowed_admission.
nh13 added a commit that referenced this pull request Aug 23, 2026
The multi-threaded pipeline serialized decode under load: at 16 threads the
decode stage ran at ~3.6x effective parallelism instead of ~15x, making
consensus/group/filter/sort 1.7x (t8) to 3.3x avg / 8.5x worst (t16) slower
than v0.6.0.

#787 (fixing the #746 deadlock) gated future-serial admission at
`heap_bytes < effective_limit / 2` with `effective_limit = min(--max-memory,
512 MiB)`. The Q3 decoded-record reorder buffer therefore stopped admitting
anything but next_seq once it held ~256 MiB -- a ceiling that scales with
neither --max-memory nor thread count; with N decode workers it is crossed
almost immediately and decode collapses to serial.

Bound Q3 admission by serial skew instead, as the reorder buffer's own
docstring prescribes: admit next_seq unconditionally (the #746 core), and a
future serial iff within [next_seq, next_seq + W) and under a raw memory
backstop, with W = clamp(4 * num_threads, .., queue_capacity). Total in-flight
memory stays bounded upstream by the Read admission gate, so decode no longer
needs a per-stage byte throttle that doubled as a parallelism cap. Scoped to
Q3 via a new `window` field (0 = legacy byte-threshold behavior).

Output is record-identical to v0.6.0; the #746 backpressure suite passes and a
tight --max-memory run stays bounded without wedging. Adds
test_reorder_buffer_state_windowed_admission.

This branch was successfully deployed

1 active deployment
github-actions — c95e22ac Deployed Aug 18, 2026 by nh13 via coverage #3647
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant