Skip to content

fix: add graceful shutdown support and prevent producer deadlock in add_messages pipeline#283

Open
KRRT7 wants to merge 6 commits into
microsoft:mainfrom
KRRT7:review-changes-pr277
Open

fix: add graceful shutdown support and prevent producer deadlock in add_messages pipeline#283
KRRT7 wants to merge 6 commits into
microsoft:mainfrom
KRRT7:review-changes-pr277

Conversation

@KRRT7

@KRRT7 KRRT7 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • feat: Wires shutdown_event through to _dispatcher_task so it stops
    accepting new work items when shutdown is signaled. Adds a drain loop in the
    else branch so the producer's put() calls can unblock and deliver the
    None sentinel without hanging.
  • fix: Breaks out of the inner chunk loop in _producer_task when
    shutdown_event is set, preventing a deadlock when the queue is full at
    shutdown time.
  • perf: Stores the first error message on MessageAssembly as it arrives
    instead of scanning all chunks at skip-time, eliminating an O(N) scan in
    _reassembler_task.
  • refactor: Extracts _is_shutdown(event) helper to replace three inline
    copies of shutdown_event is not None and shutdown_event.is_set(). Fixes
    operator precedence ambiguity in _process_one. Skips produced_messages
    increment when shutdown fires mid-chunk-loop to avoid inflating the counter
    for partially-sent messages. Changes MessageAssembly.first_error_msg to
    str | None = None so the no-error state is explicit.
  • docs: Removes a stale reference to the success property from the
    ChunkProcessingResult docstring.

Test plan

  • Trigger a shutdown while the chunk queue is full and verify the pipeline
    drains cleanly without hanging.
  • Trigger a shutdown mid-run and confirm no None sentinel is lost (i.e.
    result_queue still receives its sentinel).
  • Run with skip_failed_messages=True and confirm the correct error message
    is logged without the O(N) scan.
  • Run existing add_messages tests to confirm no regressions.

Verification notes

All four items verified by exercising the internal pipeline functions directly
through the library boundary (uv run python).

Shutdown / deadlock (items 1 & 2): ran _producer_task + _dispatcher_task
concurrently with maxsize=2 and 10 chunks. Producer confirmed blocking before
shutdown was signaled; both tasks finished within 3 s. result_queue received 5
processed results + None sentinel in the correct terminal position.

first_error_msg (item 3): fed a two-chunk message where chunk-0 raises
"first error" and chunk-1 raises "second error". Skip log line contained
only "first error" — the old O(N) scan is gone.

Regression suite (item 4): test_add_messages_pipeline.py +
test_add_messages_streaming.py45 passed in 0.14 s (after refactor commit).

Note: Running the producer without a concurrent dispatcher deadlocks
(producer blocks forever on put()), which confirms the dispatcher drain
loop is load-bearing.

KRRT7 and others added 6 commits June 9, 2026 04:56
…Result docstring

The success property was removed in a previous commit but the docstring
still referenced it, causing confusion.
Propagate shutdown_event to dispatcher for coordinated pipeline shutdown.
When shutdown is requested, skip remaining chunks instead of processing them
after the producer has stopped.
…tion

- Extract _is_shutdown(event) to replace three inline copies of the
  shutdown_event is not None and shutdown_event.is_set() pattern
- Fix operator precedence ambiguity in _process_one skip condition
- Skip produced_messages increment when shutdown fires mid-chunk-loop
  to avoid inflating the counter for partially-sent messages
- Change MessageAssembly.first_error_msg to str | None = None so the
  no-error state is explicit; read site falls back to 'Unknown error'
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