Repository navigation
Conversation
da8cae0 to
6e8a261
Compare
When the chunk-splitting path fails to deliver a chunk (network error, send returns success=False), _accumulated was cleared unconditionally, causing silent text loss. Now only the successfully-sent portion is trimmed from _accumulated; unsent text is preserved for retry.
… chunk failure - Call _send_fallback_final when unsent text remains after chunk loop - Avoid setting _final_response_sent=True when text was not fully delivered - Add test verifying gateway fallback is not suppressed on partial failure
6e8a261 to
ae4abbb
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the overflow failure path. The current-head defect is real: gateway/stream_consumer.py:668-676 clears the buffer after chunk attempts even when a send fails, and a non-final iteration continues at line 692.
Problems
gateway/stream_consumer.py:354countslen(chunk)as source progress.BasePlatformAdapter.truncate_message()appends(i/N)to multi-message chunks (gateway/platforms/base.py:5620-5625), so this can over-trim_accumulatedafter a successful chunk.gateway/stream_consumer.py:357retains the suffix while leaving_message_idon the successful chunk. A later non-final flush follows the edit path at currentgateway/stream_consumer.py:694-748, which can replace that delivered chunk with the suffix instead of sending a continuation.
Suggested changes
- Track consumed source boundaries independently of decorated transport chunks, and introduce a continuation state for partial initial-overflow delivery.
- Add a regression where failure occurs before
_DONE, later deltas arrive, and the final output contains the full text exactly once.
Automated hermes-sweeper review.
| sent_length += len(chunk) | ||
| continue | ||
| reply_id = self._message_id | ||
| new_id = await self._send_new_chunk(chunk, reply_id) |
There was a problem hiding this comment.
truncate_message() decorates multi-message chunks with (i/N) (gateway/platforms/base.py:5620-5625), so len(chunk) is not the number of source characters consumed. This slice can skip source text after a successful decorated chunk; track raw source boundaries instead.
| new_id = await self._send_new_chunk(chunk, reply_id) | ||
| if new_id is not None and new_id != reply_id: | ||
| sent_length += len(chunk) | ||
| else: |
There was a problem hiding this comment.
After a partial success, _message_id still identifies the delivered chunk. A later non-final flush will enter _send_or_edit and edit that message with this retained suffix, replacing visible content rather than creating a continuation. The partial-failure state needs an explicit continuation transition.
Summary
_send_new_chunkfails during the chunk-splitting path (network error, send returnssuccess=False),_accumulatedwas cleared unconditionally at line 193, causing silent text loss_accumulatedwere successfully delivered and only trims that portion; unsent text is preserved for retry_message_id is not Noneoverflow path (lines 207-226) which already checks_send_or_editreturn before advancing_accumulatedTest plan
test_chunk_send_failure_preserves_accumulated— all chunk sends fail →_accumulatedretains full texttest_partial_chunk_failure_preserves_unsent_text— first chunk succeeds, second fails → unsent portion preservedtest_stream_consumer.pytests still pass