-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(core): capture PTY errors in result instead of throwing from async callback #3162
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -198,12 +198,16 @@ const getErrorMessage = (error: unknown): string => | |||||||||||||
|
|
||||||||||||||
| const isExpectedPtyReadExitError = (error: unknown): boolean => { | ||||||||||||||
| const code = getErrnoCode(error); | ||||||||||||||
| if (code === 'EIO') { | ||||||||||||||
| if (code === 'EIO' || code === 'EINTR' || code === 'ENODEV') { | ||||||||||||||
| return true; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] When this error handler runs, it sets
Suggested change
— qwen3.6-plus via Qwen Code /review |
||||||||||||||
| const message = getErrorMessage(error); | ||||||||||||||
| return message.includes('read EIO'); | ||||||||||||||
| return ( | ||||||||||||||
| message.includes('read EIO') || | ||||||||||||||
| message.includes('read EINTR') || | ||||||||||||||
| (message.includes('read') && message.toLowerCase().includes('pty')) | ||||||||||||||
| ); | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| const isExpectedPtyExitRaceError = (error: unknown): boolean => { | ||||||||||||||
|
|
@@ -622,7 +626,7 @@ export class ShellExecutionService { | |||||||||||||
| let decoder: TextDecoder | null = null; | ||||||||||||||
| let output: string | AnsiOutput | null = null; | ||||||||||||||
| const outputChunks: Buffer[] = []; | ||||||||||||||
| const error: Error | null = null; | ||||||||||||||
| let error: Error | null = null; | ||||||||||||||
| let exited = false; | ||||||||||||||
|
|
||||||||||||||
| let isStreamingRawContent = true; | ||||||||||||||
|
|
@@ -812,12 +816,41 @@ export class ShellExecutionService { | |||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Surface unexpected PTY errors to preserve existing crash behavior. | ||||||||||||||
| throw err; | ||||||||||||||
| // Store the error and trigger exit handling. | ||||||||||||||
| // Throwing from an async event callback causes uncaught exception. | ||||||||||||||
| error = err; | ||||||||||||||
| if (!exited) { | ||||||||||||||
| exited = true; | ||||||||||||||
| abortSignal.removeEventListener('abort', abortHandler); | ||||||||||||||
| this.activePtys.delete(ptyProcess.pid); | ||||||||||||||
| try { | ||||||||||||||
| ptyProcess.kill(); | ||||||||||||||
| } catch { | ||||||||||||||
| // PTY may already be dead | ||||||||||||||
| } | ||||||||||||||
| // Do NOT set `error` — that field is reserved for spawn failures. | ||||||||||||||
| // Include the error message in output so downstream consumers | ||||||||||||||
| // correctly treat this as a runtime failure, not a startup failure. | ||||||||||||||
| resolve({ | ||||||||||||||
| rawOutput: Buffer.concat(outputChunks), | ||||||||||||||
| output: getErrorMessage(err), | ||||||||||||||
| exitCode: 1, | ||||||||||||||
| signal: null, | ||||||||||||||
| error: null, | ||||||||||||||
| aborted: abortSignal.aborted, | ||||||||||||||
| pid: ptyProcess.pid, | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This change starts returning unexpected runtime PTY failures through — gpt-5.4 via Qwen Code /review |
||||||||||||||
| executionMethod: | ||||||||||||||
| (ptyInfo?.name as 'node-pty' | 'lydell-node-pty') ?? 'node-pty', | ||||||||||||||
| }); | ||||||||||||||
| } | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| ptyProcess.onExit( | ||||||||||||||
| ({ exitCode, signal }: { exitCode: number; signal?: number }) => { | ||||||||||||||
| // If the error handler already resolved, skip redundant cleanup. | ||||||||||||||
| if (error) { | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
| exited = true; | ||||||||||||||
| abortSignal.removeEventListener('abort', abortHandler); | ||||||||||||||
| this.activePtys.delete(ptyProcess.pid); | ||||||||||||||
|
|
||||||||||||||
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.
[Critical]
message.includes('pty')is overly broad — will silently suppress any error containing the substring "pty", including legitimate errors like "pty allocation failed" that should be surfaced to the user.— qwen3.6-plus via Qwen Code /review