fix(client): use PilotError type check instead of fragile substring match in subscribe_event (PILOT-187)#2
Open
matthew-pilot wants to merge 1 commit into
Open
Conversation
…atch in subscribe_event (PILOT-187)
The subscribe_event loop used str(e) substring matching to detect
clean disconnects: "connection closed" or "EOF". The "EOF" branch is
too broad — any error message containing "EOF" (e.g. RuntimeError
"unexpected EOF on stream", or a hypothetical "EOFError raised
mid-frame") was silently swallowed even though the underlying error
was not a clean disconnect.
Replace the substring check with isinstance(e, PilotError) so only
the library's own PilotError instances are treated as recoverable
disconnects. The PilotError("connection closed") path (raised by the
C-bound Conn class when the underlying Go connection is closed) is the
legitimate clean-disconnect signal.
The related timeout semantics (loop timeout vs per-read deadline) are
documented in the existing docstring; no behavioral change there.
Closes PILOT-187
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What failed
subscribe_eventinclient.py:1061used fragile string matching to detect clean disconnects:The
"EOF" in str(e)branch is too broad — any error containing "EOF" (e.g.,RuntimeError("unexpected EOF on stream")) was silently swallowed as if it were a clean disconnect. This means a real connection error during event streaming would be silently lost rather than propagated.Why this fix
Replace the substring check with
isinstance(e, PilotError)so only the library's ownPilotErrorinstances are treated as recoverable disconnects. ThePilotError("connection closed")path (raised by the C-boundConnclass when the underlying Go connection closes) is the legitimate clean-disconnect signal.Changes
isinstance(e, PilotError)instead of fragile substring matchtest_eof_error_breaks_cleanly→test_eof_error_propagates—RuntimeErrorcontaining "EOF" is not a clean disconnect and should propagateVerification
uv sync --extra dev✅uv run pytest— 204 passed, 90 skipped, 100% coverage ✅Closes PILOT-187