Repository navigation
fix(realtime): surface terminal startup rejection reasons - #399
chaser-kong wants to merge 2 commits into
Conversation
|
|
tkkhq
left a comment
tkkhq
left a comment
There was a problem hiding this comment.
The inline notes are non-blocking checks for shutdown timing and retryable recovery. Per the repository rollout guidance, validate the merged SDK main revision through the coordinated Hosting staging scenario; the PR reports live acceptance as pending.
| except BaseException: | ||
| self.connection_session_lineage = None | ||
| self.connection_access_token = None | ||
| await self._discard_failed_connection(connection) |
There was a problem hiding this comment.
If transport shutdown is slow after startup times out, the caller receives the exception after more than ten seconds: cleanup is awaited outside the timeout scope. Non-blocking: add a delayed-close test to establish the caller-visible bound, since the current deadline test uses immediate cleanup.
| None | ||
| ): | ||
| events = ClientEvents(lambda _context: None) | ||
| await events.on_disconnected(SimpleNamespace(code=3001, reason="transport closed")) |
There was a problem hiding this comment.
This handler has no connection binding, so the test exercises the default false predicate rather than the production native-state check. Non-blocking: add a bound connection case that emits a retryable disconnect while connecting, then connects successfully, to protect recovery within the startup deadline.
AI review: no P0-P2 findingsMakes realtime startup wait for connection readiness or a terminal disconnect (bounded to 10s), surfaces terminal rejections as a new exported ⚪ P3
Not checked: The AI review read the diff, the code, and the PR's text; it ran no tests or builds and read nothing outside this repository.
Human review: needed
The AI review found no P0-P2 problems at 55e38ff, so the code needs no second review for bugs. Decide the points above: approve to accept them, or request changes. Suggested reviewer: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| connection: _native.VolcanoCentrifugeConnection, | ||
| ) -> None: | ||
| # Cleanup must not replace the original connection failure/cancellation. | ||
| with suppress(Exception, asyncio.CancelledError): |
There was a problem hiding this comment.
⚪ P3: Failed-connection cleanup swallows an external cancellation and can leave disconnect half-done
_discard_failed_connection wraps connection.disconnect() in suppress(Exception, asyncio.CancelledError). Suppose the task is cancelled while this cleanup runs, for example by app shutdown or a TaskGroup sibling failing after a RealtimeConnectionError or TimeoutError. The cancellation is discarded and the task ends with the original error instead of CancelledError. The native disconnect() is also interrupted midway, so the transport this PR means to release can stay open. Elsewhere the module finishes native stops under asyncio.shield and re-raises the cancellation afterwards (unsubscribe_native). Using the same pattern here would keep the original error when cleanup fails with an Exception, while still finishing the disconnect and propagating cancellation.
Found by claude-opus-5-5
| self.connection_access_token = session.access_token | ||
| try: | ||
| await connection.connect() | ||
| async with asyncio.timeout(10): |
There was a problem hiding this comment.
⚪ P3: Readiness wait holds connection_lock up to 10s, stalling disconnect() during a retrying first connect
Before this change, if the first native attempt failed with a retryable error (for example, connection refused), native connect() scheduled a reconnect and returned quickly. That released connection_lock, and the subscription's readiness wait ran outside the lock. Now _connect_locked holds the lock through events.wait_ready() for up to 10 seconds. A concurrent client.realtime.disconnect(), channel.unsubscribe() or remove_channel(), such as a shutdown handler, now blocks until the deadline passes instead of aborting the startup. A fix would be to let disconnect() cancel or supersede an in-flight startup, or to wait for readiness without holding the lock.
Found by claude-opus-5-5
| try: | ||
| # The outer deadline distinguishes the SDK's ten-second timeout from | ||
| # an absent deadline or one extended beyond its documented bound. | ||
| async with asyncio.timeout(10.75): |
There was a problem hiding this comment.
⚪ P3: Hard-coded 10s startup deadline forces a unit test that waits 10 real seconds
asyncio.timeout(10) is written inline in _connect_locked, so test_connection_startup_times_out_within_its_ten_second_deadline really sleeps about 10 seconds. That adds 10 seconds to every suite run. Under mutmut it also runs for any _connect_locked/ClientEvents mutant that faster tests don't kill. Moving the deadline into a named constant (for example in _realtime_messages, next to the other realtime limits) would let the test patch it to a small value and assert separately that the constant is 10. That keeps coverage of the documented bound without the wall-clock cost. Separately, a timeout surfaces as a bare TimeoutError() with no message, and docs/realtime.md doesn't say which exception callers should catch for it.
Found by claude-opus-5-5
| """The request failed before receiving an API response.""" | ||
|
|
||
|
|
||
| class RealtimeConnectionError(RuntimeError): |
There was a problem hiding this comment.
⚪ P3: New public RealtimeConnectionError sits outside the VolcanoError hierarchy
Every other class in errors.py ("Typed errors raised by the Volcano SDK") derives from VolcanoError, and applications that catch VolcanoError as their SDK-wide handler won't catch this one. Its code: int | None also conflicts with VolcanoError.code: str | None, so it can't be re-parented later without changing the attribute's type. Subclassing RuntimeError keeps existing except RuntimeError handlers working, which is a reasonable tradeoff. Since the class becomes public API on release, it would help to either record this decision or rename the attribute (for example disconnect_code) so the error could also inherit from VolcanoError.
Found by claude-opus-5-5
This branch has not been deployed
3 participants
Change
A terminal realtime startup rejection could return from the native connect call without a usable connection, leaving a subscription to fail later with a readiness timeout. A failed native connect also left its transport running.
Wait for connection readiness or a terminal disconnect, preserving the reason and optional numeric code in the exported
RealtimeConnectionError. Bound startup to ten seconds and dispose failed or cancelled attempts without replacing their original error. Retryable native interruptions remain recoverable. Public documentation and type-consumer coverage show how to readerror.codewithout casting.Verification
Tested Python revision:
55e38ff.3507: permission deniedpromptly and left no connected transport.go test ./scripts/cipassed. Installed wheel discovered all 59 canonical scenarios in the isolated Docker runner. Discovery and loopback checks are not live staging acceptance.Cross-language impact
Classification: public facade, shared behavior, native behavior, and public examples. No wire/OpenAPI snapshot change.
SDK-REALTIME-005— terminal startup rejection preserves the disconnect reason and numeric code, reports disconnected, and releases its transport.a80904b). Ruby: fix(realtime): preserve terminal disconnect reasons and codes volcano-sdk-ruby#352 (0d9b9f9). Hosting: https://github.com/Kong/volcano-hosting/pull/1925 (0e2fc6439).fix/realtime-connection-rejectionbranch. There are no SDK-owned active/staged copies.docs/realtime.md.connect()and Ruby usesRealtime::ClosedError. No new application API is copied between languages.