Fix marker worker result deadlock - #28
Conversation
rob-p
left a comment
There was a problem hiding this comment.
Thank you, Antonín, for tracking this down, providing the production symptom, and contributing a focused reproducer. The root-cause analysis is sound: decode and marker-resolution tasks share a worker pool, so a full native-result channel can occupy every worker while the coordinator waits exclusively for a resolution result. Draining native results during that wait is the right general correction, and the bounded scheduling horizon means those retained results do not require an unbounded channel or a second worker pool.
I recommend accepting the feature with a few changes before merge:
- The branch is based on the v0.2.0 release commit and now conflicts with
main. Commit adb1386 added another resolution wait for exact short-gap bridges, so applying the original patch mechanically would leave a new instance of the same circular-wait risk. The replacement needs to be rebased onto currentmainand route every resolution wait through the corrected mechanism. - The current patch passes the native receiver, pending map, and current native index separately through both wait helpers. Those values form one invariant and make already-wide internal signatures easier to misuse. I plan to encapsulate the receiver and ordered pending map in a small private native-result inbox, with methods that apply the stale-index rule centrally. Resolution waits will receive that inbox plus the current index.
- The multiplexing interval should be short. A native result can fill the channel immediately after a drain and before the blocking resolution receive; polling at one millisecond bounds that race without changing the normal immediate-result path.
- The concurrency test should retain the useful timeout guard while also proving that a stale native result is discarded and both current/future results are retained in index order. That tests the ordering rule introduced by the fix, not only that the worker eventually unblocks.
- After rebasing, I will run the complete workspace suite and strict Clippy, exercise large single- and multi-member marker streams (including the new pigz gap-bridge path), and compare the ordinary FASTQ hot path against current
main.
So the feature and core strategy are approved, but I would not merge the current commit as-is. Since maintainer edits are enabled, I will make these changes directly on this PR branch while preserving your authorship and contribution in the history.
|
Thank you again, Antonín. I have now pushed the revised implementation directly to this PR branch, preserving you as the author of the implementation commit. The branch is rebuilt cleanly on current
Validation completed:
GitHub now reports the PR as mergeable. There are no configured status checks attached to it, so from the implementation, correctness, and performance sides I consider this ready for final review and merge. |
Hey, thank you for this amazing crate!
When working with the crate I stumbled upon a total deadlock when decompressing some massive GZIP files.
After some back and forth with an agent we found the deadlock, the fix should is below.
The issue is both in version
0.1.0and0.2.0.Let me know if I can help in any way with getting this to master.
Thank you!
The MR description below this line and the MR itself are⚠️ vibecoded ⚠️ but I can confirm the fix works on our data and the description below aligns with my findings.
Summary
Fix a deadlock in the marker-based parallel decoding path that could occur while processing large gzip inputs, particularly concatenated gzip files under high parallelism and backpressure.
The decoder could stop making progress indefinitely: workers remained alive, but no additional compressed input was read and no decoded output was produced. Increasing the worker count or buffer sizes made the problematic scheduling state easier to reach, but decompression itself was not failing.
Root cause
Speculative DEFLATE decoding and marker resolution share the same worker pool. Their results are returned through separate bounded channels.
The deadlock occurred in this sequence:
Although the native-result channel had capacity based on the pipeline size, that capacity alone could not guarantee progress across member transitions, exact bridge decoding, adaptive worker-limit changes, and already in-flight speculative work.
This was a scheduling deadlock rather than corrupt input or a failure in the underlying DEFLATE implementation.
Fix
While waiting for the required marker-resolution result, the coordinator now also drains completed native decode results from their bounded channel.
These results are placed in the existing ordered pending map and are processed normally when their index becomes current. Results older than the current native index continue to be discarded as stale.
Draining the native-result channel:
The change does not make the result channels unbounded, create another worker pool, or alter the public API.
Why this preserves correctness
Native results drained during a resolution wait follow the same handling as results received by the normal coordinator loop:
The pending native results remain bounded by the existing scheduling horizon because the coordinator does not schedule additional speculative work while blocked in the resolution wait.
Regression test
A focused concurrency regression test recreates the circular dependency with a one-slot native-result channel:
Without the fix, the test cannot complete because the coordinator never frees the native channel. With the fix, the coordinator drains the native result, the worker proceeds to marker resolution, and both native results remain available in their correct indexed order.
Validation
The change was validated with:
rapidgzip-coretest suite;