-
Notifications
You must be signed in to change notification settings - Fork 895
fix (desktop): fix crash on input too big -> pty buffer backpressure #765
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 |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| import type * as pty from "node-pty"; | ||
|
|
||
| /** | ||
| * Non-blocking input writer for PTY. | ||
| * | ||
| * Prevents the main thread from blocking when writing large amounts of data | ||
| * (e.g., pasting Unicode text) by: | ||
| * 1. Chunking large writes into smaller pieces | ||
| * 2. Using setImmediate between chunks to yield the event loop | ||
| * | ||
| * This fixes crashes caused by synchronous PTY writes blocking when the | ||
| * PTY buffer fills up faster than the shell can consume data. | ||
| */ | ||
|
|
||
| /** | ||
| * Maximum chunk size for PTY writes (4KB). | ||
| * This is small enough to not block noticeably, but large enough | ||
| * to be efficient for normal input. | ||
| */ | ||
| const CHUNK_SIZE = 4 * 1024; | ||
|
|
||
| /** | ||
| * Threshold for using chunked writes. | ||
| * Data smaller than this is written directly (no overhead). | ||
| */ | ||
| const CHUNK_THRESHOLD = CHUNK_SIZE; | ||
|
|
||
| export class InputWriter { | ||
| private pty: pty.IPty; | ||
| private writeQueue: string[] = []; | ||
| private isWriting = false; | ||
| private isDisposed = false; | ||
|
|
||
| constructor(ptyProcess: pty.IPty) { | ||
| this.pty = ptyProcess; | ||
| } | ||
|
|
||
| /** | ||
| * Write data to the PTY without blocking the main thread. | ||
| * | ||
| * Small writes are sent directly. Large writes are chunked and | ||
| * processed asynchronously to prevent blocking. | ||
| */ | ||
| write(data: string): void { | ||
| if (this.isDisposed) { | ||
| return; | ||
| } | ||
|
|
||
| // Small data: write directly (most common case - single keystrokes) | ||
| if (data.length < CHUNK_THRESHOLD) { | ||
| try { | ||
| this.pty.write(data); | ||
| } catch (error) { | ||
| console.error("[InputWriter] Write failed:", error); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Large data: queue and process in chunks | ||
| this.writeQueue.push(data); | ||
| this.processQueue(); | ||
| } | ||
|
|
||
| /** | ||
| * Process queued writes in chunks, yielding the event loop between chunks. | ||
| */ | ||
| private processQueue(): void { | ||
| if (this.isWriting || this.writeQueue.length === 0 || this.isDisposed) { | ||
| return; | ||
| } | ||
|
|
||
| this.isWriting = true; | ||
| this.writeNextChunk(); | ||
| } | ||
|
|
||
| private writeNextChunk(): void { | ||
| if (this.isDisposed) { | ||
| this.isWriting = false; | ||
| this.writeQueue = []; | ||
| return; | ||
| } | ||
|
|
||
| // Get next chunk from front of queue | ||
| const data = this.writeQueue[0]; | ||
| if (!data) { | ||
| this.isWriting = false; | ||
| return; | ||
| } | ||
|
|
||
| // Write one chunk | ||
| const chunk = data.slice(0, CHUNK_SIZE); | ||
| const remaining = data.slice(CHUNK_SIZE); | ||
|
|
||
| try { | ||
| this.pty.write(chunk); | ||
| } catch (error) { | ||
| console.error("[InputWriter] Chunk write failed:", error); | ||
| // Remove the problematic data and continue with next | ||
| this.writeQueue.shift(); | ||
| if (this.writeQueue.length > 0) { | ||
| setImmediate(() => this.writeNextChunk()); | ||
| } else { | ||
| this.isWriting = false; | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Update queue | ||
| if (remaining.length > 0) { | ||
| // More chunks to write from this data | ||
| this.writeQueue[0] = remaining; | ||
| } else { | ||
| // This data is complete, remove from queue | ||
| this.writeQueue.shift(); | ||
| } | ||
|
|
||
| // Schedule next chunk (yields event loop) | ||
| if (this.writeQueue.length > 0) { | ||
| setImmediate(() => this.writeNextChunk()); | ||
| } else { | ||
| this.isWriting = false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Dispose of the writer, canceling any pending writes. | ||
| */ | ||
| dispose(): void { | ||
| this.isDisposed = true; | ||
| this.writeQueue = []; | ||
| this.isWriting = false; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |||||||||||||||||||||
| extractContentAfterClear, | ||||||||||||||||||||||
| } from "../terminal-escape-filter"; | ||||||||||||||||||||||
| import { buildTerminalEnv, FALLBACK_SHELL, getDefaultShell } from "./env"; | ||||||||||||||||||||||
| import { InputWriter } from "./input-writer"; | ||||||||||||||||||||||
| import type { InternalCreateSessionParams, TerminalSession } from "./types"; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const DEFAULT_COLS = 80; | ||||||||||||||||||||||
|
|
@@ -130,6 +131,8 @@ | |||||||||||||||||||||
| onData(paneId, batchedData); | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const inputWriter = new InputWriter(ptyProcess); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return { | ||||||||||||||||||||||
| pty: ptyProcess, | ||||||||||||||||||||||
| paneId, | ||||||||||||||||||||||
|
|
@@ -143,6 +146,7 @@ | |||||||||||||||||||||
| isAlive: true, | ||||||||||||||||||||||
| wasRecovered, | ||||||||||||||||||||||
| dataBatcher, | ||||||||||||||||||||||
| inputWriter, | ||||||||||||||||||||||
| shell, | ||||||||||||||||||||||
| startTime: Date.now(), | ||||||||||||||||||||||
| usedFallback: useFallbackShell, | ||||||||||||||||||||||
|
|
@@ -206,6 +210,7 @@ | |||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export function flushSession(session: TerminalSession): void { | ||||||||||||||||||||||
| session.inputWriter.dispose(); | ||||||||||||||||||||||
|
Check failure on line 213 in apps/desktop/src/main/lib/terminal/session.ts
|
||||||||||||||||||||||
| session.dataBatcher.dispose(); | ||||||||||||||||||||||
| session.headless.dispose(); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
212
to
216
Contributor
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. 🧩 Analysis chain🏁 Script executed: # First, locate the session.ts file and examine the relevant code
find . -name "session.ts" -path "*apps/desktop*" | head -5Repository: superset-sh/superset Length of output: 111 🏁 Script executed: # Examine the session.ts file to understand the flushSession function and TerminalSession type
cat -n ./apps/desktop/src/main/lib/terminal/session.ts | sed -n '1,250p'Repository: superset-sh/superset Length of output: 6719 🏁 Script executed: # Find and examine the types.ts file for TerminalSession type definition
fd "types.ts" -path "*apps/desktop/src/main/lib/terminal*" --type fRepository: superset-sh/superset Length of output: 232 🏁 Script executed: # Find and examine the types.ts file for TerminalSession type definition
fd "types.ts" -path "*apps/desktop/src/main/lib/terminal*" -type fRepository: superset-sh/superset Length of output: 232 🏁 Script executed: # Find and examine the types.ts file for TerminalSession type definition
find ./apps/desktop/src/main/lib/terminal -name "types.ts"Repository: superset-sh/superset Length of output: 109 🏁 Script executed: # Examine the types.ts file to understand TerminalSession interface
cat -n ./apps/desktop/src/main/lib/terminal/types.tsRepository: superset-sh/superset Length of output: 1849 🏁 Script executed: # Find test files related to session
find ./apps/desktop -name "*.test.ts" -o -name "*.test.tsx" | xargs grep -l "flushSession\|session" 2>/dev/null | head -10Repository: superset-sh/superset Length of output: 169 🏁 Script executed: # Examine the session.test.ts file
cat -n ./apps/desktop/src/main/lib/terminal/session.test.tsRepository: superset-sh/superset Length of output: 3677 🏁 Script executed: # Check if there's a git history or any recent changes to understand if this is a new issue
git log --oneline -20 ./apps/desktop/src/main/lib/terminal/session.tsRepository: superset-sh/superset Length of output: 98 🏁 Script executed: # Check the git diff to see what changes were made in that commit
git show 182a539 --statRepository: superset-sh/superset Length of output: 50376 🏁 Script executed: # Check the diff specifically for session.ts
git show 182a539 -- apps/desktop/src/main/lib/terminal/session.tsRepository: superset-sh/superset Length of output: 5816 🏁 Script executed: # Check if inputWriter is ever made optional or if there are other callers of flushSession
rg "flushSession" apps/desktop/src/main/lib/terminal/Repository: superset-sh/superset Length of output: 537 🏁 Script executed: # Check how flushSession is used in manager.ts
rg -A 3 -B 3 "flushSession\(session" apps/desktop/src/main/lib/terminal/manager.tsRepository: superset-sh/superset Length of output: 333 🏁 Script executed: # Check if there are any other test files for terminal/session functionality
find apps/desktop -name "*session*test*" -o -name "*test*session*" | grep -i terminalRepository: superset-sh/superset Length of output: 114 🏁 Script executed: # Check manager.test.ts for flushSession usage
rg -A 10 -B 5 "flushSession" apps/desktop/src/main/lib/terminal/manager.test.ts | head -50Repository: superset-sh/superset Length of output: 46 🏁 Script executed: # Let's look at the full test to understand what it's actually testing
sed -n '87,115p' apps/desktop/src/main/lib/terminal/session.test.tsRepository: superset-sh/superset Length of output: 662 The test mock is incomplete and will cause flushSession to fail. The test creates a partial mock of Fix the mock to include Add inputWriter to test mock const mockSession = {
dataBatcher: mockDataBatcher,
headless: mockHeadless,
+ inputWriter: { dispose: () => { /* track if called */ } },
} as unknown as TerminalSession;Alternatively, use optional chaining in 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Check: Test[failure] 213-213: TypeError: undefined is not an object (evaluating 'session.inputWriter.dispose') 🤖 Prompt for AI Agents |
||||||||||||||||||||||
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.
Unicode characters may be split mid-sequence when chunking.
String.slice()operates on UTF-16 code units, not Unicode grapheme clusters. When pasting Unicode text (which this PR specifically aims to support), slicing at arbitrary positions can split surrogate pairs or multi-byte sequences, potentially corrupting characters like emojis or CJK text.Consider aligning chunk boundaries to avoid splitting characters:
Proposed fix using a boundary-aware slice
📝 Committable suggestion