Repository navigation
fix: enforce the sync monitor timeout - #234
Merged
Merged
Conversation
monitor_operation's timeout was never applied, because connect() blocks until the stream ends. A timer now closes the stream at the timeout and the call raises TimeoutError. A close during a reconnect backoff no longer reopens the stream.
jfrench9
added a commit
that referenced
this pull request
Oct 5, 2026
…236) ## Summary Final fixes from the 2.6.0 verification pass. The main one: #234's `monitor_operation(timeout=…)` raised `TimeoutError` about 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 a `timeout`, 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 a `timeout`, the call reads the stream on the calling thread as before. - **GraphQL reads (`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 static `X-API-Key` plus a `token_provider` JWT sent both headers. - **`create_report`:** `period_start` / `period_end` are annotated `str | 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 - **New:** `test_monitor_timeout_fires_on_time_over_a_real_socket` runs a local SSE server that holds the stream open with keepalives. `timeout=1` now raises in about 1 s; without the fix the same test fails after 31 s. - **New:** two `GraphQLClient` tests 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](https://claude.com/claude-code)
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
OperationClient.monitor_operationnever applied itstimeout.connect()blocks until the stream ends, so the wait could last forever.GraphClient.materialize(default 600 s) andchange_tier(600 s, documented to raiseTimeoutError) both pass a timeout expecting it to work. This was a follow-up from #233.Changes
timeoutpasses. The read ends and the call raisesTimeoutError("Operation … timed out after Ns"), the same message as the async monitor. The timer is cancelled once the call returns, so a run that finishes in time is unaffected.SSEClientno longer reopens the stream whenclose()lands during a reconnect backoff.MonitorOptions.timeoutis nowOptional[float], in seconds. This widens the type and removes nothing.The other #233 follow-up, a
mode="stream"operator call failing, turned out not to be a defect. The server only treatsmode=syncspecially (routers/graphs/operator/execute.py).stream,asyncandautoall answer202, which #233 already follows.Compatibility
Rides a client patch. Behaviour change:
materialize()andchange_tier()now stop waiting at their timeout (600 s by default) instead of waiting forever.change_tierraisesTimeoutError, as its docstring says.materializereturns a failedMaterializationResult. The server-side operation keeps running either way.Testing
just test-all: 650 passed, 17 skipped. ruff, format and basedpyright are clean.🤖 Generated with Claude Code