feat(sdk): listener lifecycle hook for poll failures and recovery - #191
feat(sdk): listener lifecycle hook for poll failures and recovery#191fomo-ash wants to merge 1 commit into
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
devSuryansh
left a comment
There was a problem hiding this comment.
Thanks for this. I went through the listen() changes in both SDKs. The main-loop behavior looks right to me: one "reconnecting" on the first failed poll, one "connected" on recovery, nothing while polling is healthy, and a handler exception does not kill the loop. Parity between Python and TypeScript also looks good.
I left a few line notes. The one I am least sure I am wrong about is startup: default listen() (no from_seq / fromSeq) seems to retry inside _latest_seq() / latestSeq() before the new hook exists, and the tests skip that path by passing seq 0. If that is in scope for #189, it might be worth covering. The other notes are about the Python inspect.signature arity check, and the meaning of attempt on "reconnecting".
I am not a maintainer, so treat these as questions. Happy to be told I misread the startup path.
| @@ -1449,18 +1471,37 @@ def listen( | |||
| try: | |||
| seq = self._latest_seq() if from_seq is None else from_seq | |||
There was a problem hiding this comment.
I might be misreading the startup path, so take this as a question more than a finding.
listen() with no from_seq (the README default) waits on _latest_seq() before the loop that emits on_connection_state_change. _latest_seq() already polls /v1/events and retries on failure, just with a 2s sleep and no callback.
If I am reading that right, an agent that comes up while the gateway is down would retry in _latest_seq() for as long as it takes, and a health check that only flips on "reconnecting" would never hear about it. The new tests all pass from_seq=0, so I think they skip this path.
Is the cursor fetch intentionally out of scope for this hook? If not, it might be worth dispatching from both retry loops, or sharing one helper. Same shape looks present in the TypeScript latestSeq() path.
There was a problem hiding this comment.
yeah u are right here, missed this, Default listen() calls should also trigger "reconnecting" if the gateway is unreachable during initial cursor resolution. thanks!
| @@ -1388,17 +1416,33 @@ export class CommClient { | |||
| try { | |||
| let seq = opts.fromSeq ?? (await this.latestSeq(opts.signal)); | |||
There was a problem hiding this comment.
Same question as on the Python listen() / _latest_seq() path, in case it is easier to discuss next to this line.
If fromSeq is omitted, latestSeq() retries /v1/events on its own before currentState and dispatchConnectionState exist. I could be missing a reason to treat startup cursor fetch as different from the poll loop. If there isn't one, the hook as written might not fire for the default client.listen() call while the gateway is unreachable.
The TypeScript tests also pass fromSeq: 0, so I think they miss this too.
There was a problem hiding this comment.
inspecting parameter length was too brittle for variadic or keyword only signatures. replacing this with an invocation fallback
try:
self._call_handler(handler, state, detail)
except TypeError:
self._call_handler(handler, state)please let me know if u have other suggestions
| sig = inspect.signature(handler) | ||
| if len(sig.parameters) == 1: | ||
| self._call_handler(handler, state) | ||
| else: | ||
| self._call_handler(handler, state, detail) |
There was a problem hiding this comment.
I am not sure this arity check does what we want.
len(inspect.signature(handler).parameters) == 1 is not the same as "this function takes one positional argument." Something like def handle(state, **kwargs): has two parameters, so this branch would call handler(state, detail) and TypeError. That exception is caught just below, so the handler would fail closed and look like a no-op. Keyword-only second args would hurt the same way.
TypeScript always passes (state, detail), which seems simpler. Would it be reasonable to do that here too, and drop the inspect branch? Or bind with signature.bind and fall back. I did not see a test for a one-arg handler or for **kwargs, so I may also just be missing the intended contract.
| { | ||
| "error": err, | ||
| "backoff_seconds": backoff, | ||
| "attempt": failed_attempts, |
There was a problem hiding this comment.
Tiny API question, not sure it matters.
"reconnecting" only fires on the first failure, and failed_attempts is incremented in this same block, so attempt here will always be 1. On "connected" it is the full failure count. If someone uses detail["attempt"] as "how many times have we failed," they will keep seeing 1 until recovery.
If that is intentional, it might help to say so in the docstring. If not, omitting attempt on "reconnecting" (or only setting it on "connected") could avoid the mixed meaning. Same on the TypeScript side with attempt: failedAttempts.
What & why
Closes #189
Adds an
onConnectionStateChange/on_connection_state_changelifecycle hook to both the TypeScript and Python SDKs.Currently, when
listen()encounters a poll failure (network blip, gateway 5xx), it logs a warning and retries with exponential backoff silently. This PR provides a callback hook that notifies application code when polling transitions into error backoff ("reconnecting") and when it successfully recovers ("connected").Key Features & Semantics:
"reconnecting"once on the first failure transition (not on every retry attempt while already reconnecting)."connected"once upon successful recovery with the count of failed attempts.listen().Checklist
uv run pytestgreen (62 passed)uv run ruff check .cleancd sdks/typescript && npm test && npm run typecheck(61 passed, 0 errors)