Skip to content

fix(dump): drain pending data in FIFO writer on shutdown#71

Merged
nick-youngblut merged 2 commits into
mainfrom
fix/fifo-writer-shutdown-data-loss
Jun 8, 2026
Merged

fix(dump): drain pending data in FIFO writer on shutdown#71
nick-youngblut merged 2 commits into
mainfrom
fix/fifo-writer-shutdown-data-loss

Conversation

@nick-youngblut

@nick-youngblut nick-youngblut commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a data-integrity race in the multi-threaded dump writer where pending bytes could be silently dropped when the buffered FIFO writer shut down, truncating FASTA/FASTQ output (most visibly over named pipes).

Changes

  • Replace ThreadWriter's dual signaling (an mpsc shutdown channel plus a Condvar over the buffer) with a single mutex-guarded WriterState { buffer, closed } driven by one condition predicate.
  • The worker thread now exits only when the buffer is empty and closed is set; a non-empty buffer always takes the drain branch, so all ingested bytes are flushed before the thread exits.
  • Drop sets closed = true under the lock before notifying, closing the wakeup gap, then joins the worker.
  • ingest() adapted to the new state struct; backpressure behavior is unchanged.
  • Add in-memory regression tests, including a 1000-iteration stress loop that ingests immediately before drop to surface the race.

ThreadWriter coordinated data and shutdown through two independent
mechanisms (an mpsc channel for shutdown and a Condvar over the buffer).
The worker re-checked the shutdown channel right after waking from
cvar.wait and could return while bytes were still buffered, silently
truncating FASTA/FASTQ output (notably over named pipes).

Collapse the buffer and a `closed` flag into a single mutex-guarded
WriterState driven by one condition predicate. The worker now exits only
when the buffer is empty AND closed, so a non-empty buffer always drains
before the thread exits. Drop sets `closed` under the lock before
notifying, closing the wakeup gap.

Adds in-memory regression tests including a 1000-iteration stress loop.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors ThreadWriter to resolve a data-loss race condition during shutdown by introducing a shared WriterState struct that manages both the write buffer and the shutdown state under a single mutex and condition variable. Additionally, new unit tests are added to verify proper draining of pending data on drop. The review feedback highlights two key areas for improvement: first, a critical issue where a mutex lock is held during a sleep call in the ingest loop, which blocks the worker thread; and second, a performance optimization to reuse the write buffer's allocated capacity instead of discarding it on every write cycle.

Comment thread src/dump/output.rs
Comment thread src/dump/output.rs
Updated ThreadWriter to release the lock before sleeping, allowing the worker thread to drain the buffer during backpressure. This change improves the efficiency of data handling during shutdown.
@nick-youngblut nick-youngblut merged commit dd7785d into main Jun 8, 2026
9 checks passed
@nick-youngblut nick-youngblut deleted the fix/fifo-writer-shutdown-data-loss branch June 8, 2026 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant