-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: Improve stream error handling and parsing #5807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This commit improves the robustness of stream processing in the llamacpp-extension. - Adds explicit handling for 'error:' prefixed lines in the stream, parsing the contained JSON error and throwing an appropriate JavaScript Error. - Centralizes JSON parsing of 'data:' and 'error:' lines, ensuring consistent error propagation by re-throwing parsing exceptions. - Ensures the async iterator terminates correctly upon encountering stream errors or malformed JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 49af64f in 2 minutes and 22 seconds. Click for details.
- Reviewed
52
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/src/index.ts:974
- Draft comment:
Consider wrapping JSON.parse in the error branch with try-catch to provide clearer error context if malformed JSON is received. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_RpOAUJItZZktvF3a
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 35.1%Your code coverage diff: -0.01% ▾ ✅ All code changes are covered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed fa7fe8b in 2 minutes and 29 seconds. Click for details.
- Reviewed
36
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/src/index.ts:875
- Draft comment:
Removed the console.log(sInfo) call. This cleanup helps avoid leaking session info. If logging is needed, consider using a proper debug-level logger. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. extensions/llamacpp-extension/src/index.ts:969
- Draft comment:
Removed the console.log(trimmedLine) and console.log(jsonStr) debug statements in the streaming handler. This cleans up the output; ensure that any needed debugging info uses a proper logging strategy. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is purely informative, explaining what was done without suggesting any changes or asking for confirmation. It doesn't align with the rules for good comments, as it doesn't provide a specific suggestion or ask for a test.
3. extensions/llamacpp-extension/src/index.ts:976
- Draft comment:
The new else branch that throws 'Malformed chunk' improves robustness by handling unexpected streamed data. Consider including the actual chunk data (with caution) for better diagnostics if appropriate. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The suggestion to include chunk data could help with debugging, but there are risks: 1) The chunk could contain sensitive data 2) The chunk could be very large 3) The error is for an edge case that "should not normally reach here". The current simple error is likely sufficient since this is an unexpected edge case. The comment correctly identifies an opportunity for better error diagnostics. Including the problematic data could help track down issues if they occur in production. While better diagnostics are good, the risks of exposing potentially sensitive or large chunks of data outweigh the debugging benefits for this edge case that should rarely occur. The comment should be deleted. The current simple error message is appropriate for this edge case, and including chunk data could expose sensitive information.
Workflow ID: wflow_ua94S8uAeUxnpzPN
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
This PR significantly enhances the robustness and reliability of stream processing within the
llamacpp-extension
. Previously, certain errors transmitted via the streaming API from thellamacpp
server might not have been correctly interpreted or propagated, leading to silent failures or unexpected behavior in the client.Changes
error:
. These lines are now parsed as JSON, and the contained error message is used to throw a proper JavaScriptError
, ensuring that server-side errors are immediately recognized and dealt with.data:
anderror:
prefixed lines has been unified. This ensures consistency and simplifies the code.console.log
statement related tois_process_running
has been removed, cleaning up the console output.Why these changes?
Further work:
Important
Improves error handling and JSON parsing in
llamacpp-extension
'shandleStreamingResponse
function, ensuring robust error propagation and removing a debug log.handleStreamingResponse
inindex.ts
now explicitly handles lines starting witherror:
by parsing them as JSON and throwing a JavaScriptError
.data:
anderror:
lines inhandleStreamingResponse
.console.log
related tois_process_running
inindex.ts
.This description was created by
for fa7fe8b. You can customize this summary. It will automatically update as commits are pushed.