fix: give up on a monitored operation at its timeout, not 30s later - #236
Merged
Merged
Conversation
Closing the stream from a timer thread did not wake the blocked socket read, so monitor_operation raised only when the stream's own 30s read timeout ran out. The stream is now read on a worker thread and the timeout is kept by the caller. GraphQL reads send exactly one credential, as the REST writes do, and create_report's date parameters are annotated as the str or date they accept.
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.
Summary
Final fixes from the 2.6.0 verification pass. The main one: #234's
monitor_operation(timeout=…)raisedTimeoutErrorabout 30 seconds late. Its timer closed the stream from another thread, but that does not wake a socket read that is already blocked. The read only returned when the stream's own 30-second read timeout ran out, even with the server sending keepalives. #234's tests replaced the stream with a mock, so they never touched a real socket.Changes
OperationClient.monitor_operation: with atimeout, the stream is read on a worker thread, and the caller waits on an event set by a terminal event or by the stream ending. The timeout now fires on time. A finished run returns at once, even if the server holds the socket open after the terminal event. Without atimeout, the call reads the stream on the calling thread as before.GraphQLClient): the resolved token now replaces any credential in the static headers, so exactly one is sent, as the REST writes already do. Before, a staticX-API-Keyplus atoken_providerJWT sent both headers.create_report:period_start/period_endare annotatedstr | datetime.date, which is what they already accepted.Compatibility
Rides in the next client release with #235. That release is a minor (2.7.0) because #235 adds a facade parameter. This PR changes no signatures; the annotation change only widens a type.
Testing
test_monitor_timeout_fires_on_time_over_a_real_socketruns a local SSE server that holds the stream open with keepalives.timeout=1now raises in about 1 s; without the fix the same test fails after 31 s.GraphQLClienttests check that exactly one credential is sent. Both fail without the fix.just test-all: 654 passed, 17 skipped; ruff, format and basedpyright are clean.🤖 Generated with Claude Code