fix(clients): pipe errors (Python + TS), SSE rollback, getTaskStatus timeout#704
fix(clients): pipe errors (Python + TS), SSE rollback, getTaskStatus timeout#704anantteotia wants to merge 16 commits intorocketride-org:developfrom
Conversation
Made-with: Cursor
anantteotia
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughData pipe lifecycle methods now raise Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
No description provided. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/client-python/src/rocketride/mixins/data.py (1)
242-278:⚠️ Potential issue | 🟡 MinorDocument the new
PipeExceptionraised byclose().
close()now raisesPipeExceptionon server failure (line 278), but its docstring (lines 242-258) has noRaises:section at all. Add one so callers know the close path can throw — relevant becausesend()at lines 408-420 swallows the original exception only if a follow-uppipe.close()cleanup also fails, but external callers usingpipedirectly need to know.📝 Suggested docstring addition
Returns: Dict: Results from processing the data you sent + Raises: + PipeException: If the server reports a failure while + finalizing the pipe. + Example:Also worth aligning the
send()docstring at line 375 (RuntimeError: If sending fails) with the new behavior in a follow-up — failures from the underlying pipe will now propagate asPipeException.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client-python/src/rocketride/mixins/data.py` around lines 242 - 278, Update the docstring for the async method close() to include a Raises: section that documents PipeException being raised when the server indicates failure (i.e., after did_fail(response) triggers the PipeException constructed from response), and mention the method returns PIPELINE_RESULT on success; also update the send() docstring (method send) to note that underlying pipe failures now propagate as PipeException instead of only RuntimeError to keep docs consistent with runtime behavior. Ensure you reference the PipeException symbol and the close() and send() methods in the docstrings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client-python/src/rocketride/mixins/data.py`:
- Around line 236-240: Update the write() docstring to accurately list the two
distinct exceptions: keep RuntimeError for the precondition "pipe not opened"
and replace the generic "write fails" RuntimeError wording with PipeException
for write failures; reference the write() method and PipeException class so the
doc clearly documents "Raises: RuntimeError: if pipe not opened" and
"PipeException: if the write operation fails (response indicates failure)".
- Around line 176-187: Update the docstring for the pipe-opening method to list
both RuntimeError (for pre-condition "pipe already opened") and PipeException
(for failures returned by self._client.did_fail) so callers know what to catch,
and change the pre-condition that currently raises RuntimeError('Pipe already
opened') to raise PipeException (or a dedicated PipeAlreadyOpened subclass)
instead so both pre-condition and server-side failures use the same exception
family (reference: PipeException, RocketRideException/DAPException hierarchy,
self._client.did_fail, and the code that sets response['message']).
---
Outside diff comments:
In `@packages/client-python/src/rocketride/mixins/data.py`:
- Around line 242-278: Update the docstring for the async method close() to
include a Raises: section that documents PipeException being raised when the
server indicates failure (i.e., after did_fail(response) triggers the
PipeException constructed from response), and mention the method returns
PIPELINE_RESULT on success; also update the send() docstring (method send) to
note that underlying pipe failures now propagate as PipeException instead of
only RuntimeError to keep docs consistent with runtime behavior. Ensure you
reference the PipeException symbol and the close() and send() methods in the
docstrings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1083e59-b4c5-44e2-bb14-f93f8493d0e9
📒 Files selected for processing (1)
packages/client-python/src/rocketride/mixins/data.py
Made-with: Cursor
Made-with: Cursor
|
Good catch — agreed this would be a behavior change for users catching only RuntimeError. I pushed a follow-up commit that preserves backwards compatibility by making PipeException also inherit from RuntimeError (class PipeException(RocketRideException, RuntimeError)), so existing except RuntimeError: handlers keep working while callers can still catch PipeException specifically. PR updated accordingly. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/client-python/src/rocketride/core/exceptions.py (1)
161-188: 🧹 Nitpick | 🔵 TrivialLGTM — intentional widening for backward compatibility.
Adding
RuntimeErrorto the bases is consistent with the PR goal: code paths inmixins/data.py(open()/write()/close()) previously raised a bareRuntimeError, so existingexcept RuntimeError:handlers continue to work after the switch toPipeException. The MRO is valid (PipeException → RocketRideException → DAPException → RuntimeError → Exception) andDAPException.__init__'ssuper().__init__(error_message)chain remains compatible sinceRuntimeError.__init__accepts the same string arg.One minor nit: the class docstring (lines 162–186) doesn't mention the dual inheritance. Worth a one‑liner so users grepping the exceptions module understand that
except RuntimeErrorwill also catch this — and conversely that broadexcept RuntimeErrorhandlers will now swallow pipe errors too.📝 Optional docstring tweak
class PipeException(RocketRideException, RuntimeError): """ Exception raised for data pipe operations. Raised when there are problems with data pipes used for sending data to pipelines, uploading files, or streaming operations. + + Note: + Also inherits from :class:`RuntimeError` for backward compatibility + with callers that previously caught ``RuntimeError`` from + ``client.send()`` / ``client.pipe()``.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client-python/src/rocketride/core/exceptions.py` around lines 161 - 188, Update the PipeException docstring to explicitly note its dual inheritance (inherits from RocketRideException and RuntimeError) so users know that "except RuntimeError:" will also catch PipeException (and that broad RuntimeError handlers may swallow pipe errors); reference PipeException and the mixins/data.py methods open()/write()/close() in the sentence so readers can connect the change that preserved backward compatibility with existing except RuntimeError handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/client-python/src/rocketride/core/exceptions.py`:
- Around line 161-188: Update the PipeException docstring to explicitly note its
dual inheritance (inherits from RocketRideException and RuntimeError) so users
know that "except RuntimeError:" will also catch PipeException (and that broad
RuntimeError handlers may swallow pipe errors); reference PipeException and the
mixins/data.py methods open()/write()/close() in the sentence so readers can
connect the change that preserved backward compatibility with existing except
RuntimeError handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5c337a9-9ca9-42d2-8c5c-9ad4fd23c3b3
📒 Files selected for processing (1)
packages/client-python/src/rocketride/core/exceptions.py
|
Addressed the nit: added a short note to the PipeException docstring explaining it also inherits from RuntimeError for backward compatibility with callers catching RuntimeError from client.send() / client.pipe() (and pipe open/write/close). |
Made-with: Cursor
|
How about applying the same logic to typescript client? |
Made-with: Cursor
|
Applied the same improved data-pipe error context to the TypeScript client (DataPipe open/write/close now throw PipeException with the common-causes context on open). Pushed in commit 39e6865 as part of this PR. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/client-typescript/src/client/client.ts (1)
137-155:⚠️ Potential issue | 🟡 MinorWrap the SSE subscription failure path in the same exception type.
open()now convertsdidFail(response)intoPipeException, but the optionalsetEvents()call at the end of the success path still throws a genericError. If SSE registration fails, callers lose the new actionable context and the exception type no longer matches the rest of the pipe lifecycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client-typescript/src/client/client.ts` around lines 137 - 155, open() currently converts a failed response into a PipeException but leaves the SSE registration (this._client.setEvents) to throw a generic Error; wrap the await this._client.setEvents(...) call in a try/catch and on any error throw a PipeException instead, preserving useful context (e.g., merge the original response or construct a response-like object and include the caught error message) so callers receive a PipeException consistent with the rest of the pipe lifecycle; update the code paths that set this._client._ssePipeCallbacks only after successful setEvents, and reference the same symbols: open(), this._client.setEvents, this._onSSE, this._pipeId, and PipeException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/client-typescript/src/client/client.ts`:
- Around line 137-155: open() currently converts a failed response into a
PipeException but leaves the SSE registration (this._client.setEvents) to throw
a generic Error; wrap the await this._client.setEvents(...) call in a try/catch
and on any error throw a PipeException instead, preserving useful context (e.g.,
merge the original response or construct a response-like object and include the
caught error message) so callers receive a PipeException consistent with the
rest of the pipe lifecycle; update the code paths that set
this._client._ssePipeCallbacks only after successful setEvents, and reference
the same symbols: open(), this._client.setEvents, this._onSSE, this._pipeId, and
PipeException.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a2d4bdf-bfac-4910-bfba-2f619958c5a9
📒 Files selected for processing (1)
packages/client-typescript/src/client/client.ts
Made-with: Cursor
|
Addressed CodeRabbit TS note: wrapped DataPipe.open() SSE subscription (setEvents) failures to throw PipeException (and only register _ssePipeCallbacks after successful setEvents). Pushed in commit 949a595. |
|
@anantteotia many thanks for the update. It's almost fine and only needs conflict resolution. |
Made-with: Cursor
|
Resolved merge conflicts against latest develop and pushed updated branch (merge commit 64a9451). PR should be mergeable now. |
Made-with: Cursor
|
CI failures were due to ROCKETRIDE_APIKEY secret being unavailable on fork PRs (tests boot local server requiring shared key). Added a fallback to 'MYAPIKEY' in .github/workflows/_build.yaml so fork PR CI can pass. Commit 7cff80e. |
Made-with: Cursor
|
Ubuntu CI failed because \getTaskStatus()\ could hang indefinitely (no per-request timeout), causing \Pipeline Operations → should get pipeline status\ to hit Jest's timeout under parallel CI load.\n\nFix:\n- \getTaskStatus\ now passes \ imeout: 15000\ to \call()/request()\n- Integration test uses \TEST_CONFIG.timeout\ + slightly more retries\n\nPushed: a6671f6 |
This reverts commit 7cff80e.
Head branch was pushed to by a user without write access
|
Reverted the CI workflow change as requested (removed the fork-PR ROCKETRIDE_APIKEY fallback). Commit: 495dba5. If you'd like, I can open a separate PR to address fork-PR secret unavailability in CI in a safer/approved way. |
|
@stepmikhaylov The CI/CD workflow change has been reverted (commit 495dba5). Once current CI finishes, could you please re-review / mark the ‘changes requested’ as resolved? Thanks! |
Made-with: Cursor
|
CI is failing on fork PRs with AuthenticationException: Not authenticated because ROCKETRIDE_APIKEY isn't set when the local test server is spawned. Per request to avoid CI/CD workflow edits, I fixed this in the test harness instead: client-python and client-typescript test tasks now pass env ROCKETRIDE_APIKEY=(existing env or 'MYAPIKEY') into startServer(). Commit: 63fea26. |
github-actions
Bot
added
builder
and removed
module:client-python
Made-with: Cursor
|
CI run 25185191220 shows ROCKETRIDE_APIKEY is set to an empty string in the job env, and connect() was using nullish-coalescing (??), so it preferred the blank env key over the constructor auth, sending an empty credential and causing AuthenticationException: Not authenticated.\n\nFix: treat blank ROCKETRIDE_APIKEY in env snapshot as unset, so connect() falls back to this._apikey. Commit: b120203. |
Made-with: Cursor
|
CI run 25186225146 still failing on python client tests with AuthenticationException because ROCKETRIDE_APIKEY is present-but-empty in CI env; pytest uses os.getenv('ROCKETRIDE_APIKEY', 'MYAPIKEY') (empty string does not fall back). Fixed by forcing ROCKETRIDE_APIKEY=process.env.ROCKETRIDE_APIKEY||'MYAPIKEY' in client-python test task env before running pytest. Commit: 6ba2b13. |
asclearuc
changed the title
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thanks @anantteotia — direction is right and the typed PipeException upgrade is a clear win.
Requesting changes — two blocking items (a half-open pipe state leak in TS open() and a PR-title scope mismatch), plus a handful of smaller things.
See inline comments for per-line detail and suggested edits.
- DataPipe: set _opened only after successful SSE setup; rollback via close() on setEvents failure; close() when server assigned pipe_id even if not marked opened
- getTaskStatus: default 15s per-call timeout; optional override or { timeout: false }
- connect: ignore blank ROCKETRIDE_APIKEY in env snapshot (documented)
- Changelog: note TS client behavior changes for upgraders
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
anantteotia
changed the title
|
@asclearuc @stepmikhaylov CI is green on the latest commits. Review items are implemented (DataPipe SSE rollback + \close()\ after partial open, \getTaskStatus\ timeout API with default 15s and { timeout: false }, blank \ROCKETRIDE_APIKEY\ handling). I updated the PR title and description (including the env-var behavior note) and CHANGELOG [Unreleased]. Please re-review when you can. |
3 participants
What this does
Improves Python SDK pipe errors for
send()/pipe()by raisingPipeExceptionwith actionable context (common causes + underlying server message) instead of genericRuntimeError, while keeping backward compatibility (PipeExceptionalso inheritsRuntimeError).Aligns the TypeScript client:
DataPipeopen/write/close throwsPipeExceptionwith the same style of hints;getTaskStatususes a bounded default wait;connect()ignores blankROCKETRIDE_APIKEYin the env snapshot so constructor auth is not wiped.Test harnesses pass a shared default
ROCKETRIDE_APIKEYfor fork PRs where repo secrets are unavailable (without changing the CI workflow).Why
Pipe failures were too generic to debug; TS had a half-open risk if SSE subscribe failed after the server opened the pipe; CI could hang or mis-auth when env keys were empty.
How to test
client.send(...)with an invalid token and verify aPipeExceptionwith helpful hints.client.send(...)against a chat-source pipeline and verify the error mentions usingclient.chat().Notes
Client-side error handling only; no protocol changes.
Notes for reviewers
setEventsfails after the server assigns apipe_id, the client best-effortsclose()and only sets_openedafter SSE setup + callback registration succeed (avoids half-open state).close()can run when apipe_idexists even if_openedwas never set.getTaskStatus: Default per-call timeout 15s; optional second argumentoptionswithtimeoutin ms, or{ timeout: false }to omit the per-call override (falls back to normal client request timeout behavior). SeeCHANGELOG.md[Unreleased].connect/ env: Empty or whitespace-onlyROCKETRIDE_APIKEYin the client env snapshot is treated as unset (behavior change vs treating""as a credential); documented in CHANGELOG.Summary by CodeRabbit
Bug Fixes
Documentation